Skip to content

Conversation

@adwk67
Copy link
Member

@adwk67 adwk67 commented May 3, 2024

Description

fixes #516.

Definition of Done Checklist

  • Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
  • Please make sure all these things are done and tick the boxes
# Author - [x] Link to hdfs-utils repo - [x] Link to integration tests folder - [x] Provide a short example and explanation of the example 
# Reviewer - [ ] Code contains useful comments - [ ] Code contains useful logging statements - [ ] (Integration-)Test cases added - [ ] Documentation added or updated. Follows the [style guide](https://docs.stackable.tech/home/nightly/contributor/docs-style-guide). - [ ] Changelog updated - [ ] Cargo.toml only contains references to git tags (not specific commits or branches) 
# Acceptance - [ ] Feature Tracker has been updated - [ ] Proper release label has been added - [ ] [Roadmap](https://github.com/orgs/stackabletech/projects/25/views/1) has been updated 
@adwk67 adwk67 marked this pull request as ready for review May 3, 2024 13:22
sbernauer
sbernauer previously approved these changes May 6, 2024
Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This exceeds what I expected to document, so many thanks :)

I'm not 100% sure if we want to link to the rego rules in hdfs-utils and integration tests. I think it's a bit easier to only have a single source of truth - which needs to be the integration test to align with trino-operator

Co-authored-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@adwk67
Copy link
Member Author

adwk67 commented May 6, 2024

This exceeds what I expected to document, so many thanks :)

I'm not 100% sure if we want to link to the rego rules in hdfs-utils and integration tests. I think it's a bit easier to only have a single source of truth - which needs to be the integration test to align with trino-operator

Yes, good point. I'd quite like to keep the reference to hdfs_test.rego in there, which refers to the rego rule in hdfs-utils. I can maybe just use it in-line and remove the link to hdfs-utils. What do you think?

@sbernauer
Copy link
Member

Ahh I see the point of including the test 👍 Yep, that suggestion sounds good to me 👍

fhennig
fhennig previously requested changes May 8, 2024
@sbernauer
Copy link
Member

sbernauer commented May 10, 2024

Yes, good point. I'd quite like to keep the reference to hdfs_test.rego in there, which refers to the rego rule in hdfs-utils. I can maybe just use it in-line and remove the link to hdfs-utils. What do you think?

Thinking about this, in trino-operator we have the rego rules and the tests. We could copy/paste the approach to hdfs-operator, so that we can link to rego rules and tests with a single link.

I can volunteer to copy the setup from trino to hdfs, pls ping me if I should do it in this branch

adwk67 and others added 3 commits May 13, 2024 09:53
Co-authored-by: Felix Hennig <fhennig@users.noreply.github.com>
Co-authored-by: Felix Hennig <fhennig@users.noreply.github.com>
Co-authored-by: Felix Hennig <fhennig@users.noreply.github.com>
@adwk67
Copy link
Member Author

adwk67 commented May 13, 2024

I can volunteer to copy the setup from trino to hdfs, pls ping me if I should do it in this branch

That would be great @sbernauer : thank you!

Copy link
Member

@sbernauer sbernauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can volunteer to copy the setup from trino to hdfs, pls ping me if I should do it in this branch

After spending quite some time on it I think I would drop this. In comparison to trino we need to a.) run jinja template and b.) substitute the namespace when applying.
When doing all of this we loose the benefit of being to opa test it...

So I would say it's totally fine to link to the integration test for the source of truth of rego rules (they are nightly tested) and link to hdfs-utils for the rego rule tests in case someone is interested

@adwk67 adwk67 dismissed fhennig’s stale review May 16, 2024 14:14

requested changes are implemented

@adwk67 adwk67 added this pull request to the merge queue May 16, 2024
Merged via the queue into main with commit c2c79fe May 16, 2024
@adwk67 adwk67 deleted the docs/doc-rego-rules branch May 16, 2024 14:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

4 participants