Skip to content

Add support for object-templates-raw #158

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 22, 2024
Merged

Add support for object-templates-raw #158

merged 1 commit into from
Apr 22, 2024

Conversation

@JeffeyL
Copy link
Contributor

@JeffeyL JeffeyL commented Apr 11, 2024

Adds support for manifest files with only object-templates-raw field, which gets put into a ConfigurationPolicy.

Ref: https://issues.redhat.com/browse/ACM-10565

@openshift-ci openshift-ci bot requested review from dhaiducek and gparvin April 11, 2024 18:26
@JeffeyL JeffeyL force-pushed the ACM-10565 branch 4 times, most recently from c1ed5d8 to 49f27aa Compare April 11, 2024 19:03
dhaiducek
dhaiducek reviewed Apr 11, 2024
View reviewed changes
@JeffeyL JeffeyL marked this pull request as draft April 11, 2024 19:56
@JeffeyL JeffeyL marked this pull request as ready for review April 15, 2024 17:24
@openshift-ci openshift-ci bot requested a review from dhaiducek April 15, 2024 17:24
dhaiducek
dhaiducek reviewed Apr 15, 2024
View reviewed changes
@JeffeyL JeffeyL requested a review from dhaiducek April 15, 2024 20:31
dhaiducek
dhaiducek requested changes Apr 15, 2024
View reviewed changes
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

Looking good!

Unless I missed it, could you also add some additional parameters to the tests, like extraDependencies, ignorePending, or evaluationInterval, to make sure those are getting set properly?

mprahl
mprahl reviewed Apr 16, 2024
View reviewed changes
mprahl
mprahl reviewed Apr 16, 2024
View reviewed changes
internal/utils_test.go
tmpDir := t.TempDir()
manifestPath := path.Join(tmpDir, "configmap.yaml")
manifestYAML := `
object-templates-raw: |
Copy link
Member

@mprahl mprahl Apr 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you have a test to verify when a single manifest file has two object-templates-raw fields? For example:

object-templates-raw: |
  something
---
object-templates-raw: |
  something2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up just making the test have two object-templates-raw fields instead of one. Would it still be necessary to test the singular case?

Copy link
Member

Choose a reason for hiding this comment

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

I think one test with both in it is fine.

@JeffeyL JeffeyL force-pushed the ACM-10565 branch 2 times, most recently from 2d537df to a13315c Compare April 16, 2024 19:46
@JeffeyL JeffeyL requested a review from dhaiducek April 16, 2024 19:48
mprahl
mprahl reviewed Apr 16, 2024
View reviewed changes
mprahl
mprahl reviewed Apr 16, 2024
View reviewed changes
mprahl
mprahl reviewed Apr 16, 2024
View reviewed changes
mprahl
mprahl previously approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@mprahl mprahl left a comment

Choose a reason for hiding this comment

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

Nice!

/hold in case @dhaiducek wants to take another look.

dhaiducek
dhaiducek requested changes Apr 17, 2024
View reviewed changes
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

One more thing I thought of: for due diligence, please also add one configuration to the examples/ directory and also add to the manifests description that we've enabled this:

policy-generator-plugin/docs/policygenerator-reference.yaml

Line 215 in a097bd7

# Supported manifests:

@openshift-ci openshift-ci bot removed the lgtm label Apr 17, 2024
@openshift-ci openshift-ci bot removed the approved label Apr 17, 2024
@JeffeyL JeffeyL requested a review from dhaiducek April 17, 2024 14:33
mprahl
mprahl reviewed Apr 17, 2024
View reviewed changes
@JeffeyL
Add support for object-templates-raw
f339a95 
Adds support for manifest files with only object-templates-raw field, which gets
put into a ConfigurationPolicy.

Signed-off-by: Jeffrey Luo <[email protected]>
@mprahl
Copy link
Member

mprahl commented Apr 22, 2024

@dhaiducek let me know if this is good from your perspective to unhold.

dhaiducek
dhaiducek approved these changes Apr 22, 2024
View reviewed changes
Copy link
Member

@dhaiducek dhaiducek left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!
The object-templates-raw example doesn't actually have any templates in it, but I think that's fine--if anyone comes looking around in the generator, they're probably already familiar with ConfigurationPolicy anyway.

@openshift-ci
Copy link

openshift-ci bot commented Apr 22, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhaiducek, JeffeyL, mprahl

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@dhaiducek
Copy link
Member

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit bbbc248 into open-cluster-management-io:main Apr 22, 2024
@magic-mirror-bot magic-mirror-bot bot mentioned this pull request Apr 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhaiducek dhaiducek dhaiducek approved these changes

@gparvin gparvin Awaiting requested review from gparvin

+1 more reviewer

@mprahl mprahl mprahl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@mprahl mprahl

@dhaiducek dhaiducek

Labels

approved dco-signoff: yes lgtm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JeffeyL @mprahl @dhaiducek