Skip to content

Handled sentinel policy scenario for copywrite header #119

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.

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

Merged
6 commits merged into from
Feb 20, 2025
Merged

Conversation

ghost
Copy link

@ghost ghost commented Feb 11, 2025

🛠️ Description

We need to skip sentinel policy text before adding the copywrite header. Added regex to match the pattern for sentinel policy and skip the lines

Added new function to match regex patterns which can be skipped while running on files

🔗 External Links

https://hashicorp.atlassian.net/browse/IND-2193

👍 Definition of Done

  • New functionality works?
  • Tests added?

🤔 Can be merged upon approval?

@ghost
Copy link
Author

ghost commented Feb 11, 2025

Able to build the branch on local, the build executrables failed because of depreciated upload-artifact action

@ghost ghost marked this pull request as ready for review February 11, 2025 06:43
@ghost ghost self-requested a review as a code owner February 11, 2025 06:43
Copy link
Member

@CalebAlbers CalebAlbers left a comment

Choose a reason for hiding this comment

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

Hey there! Could I ask for a bit more context on why Sentinel policies need to be excluded? It feels like this change will only skip copyright headers on some sentinel policies, but not others.

I don't have the full context on whether Sentinel policies need/can have copyright headers attached. I will mention that if it is due to things like auto-generated documentation getting the copyright header, too, Copywrite has the ability to add a .copywrite.hcl config that excludes individual files, whole folders, or other search globs.

Depending on the use case, there may be more robust mechanisms for achieving the end result without making assumptions inside the code that other parties have to contend with (e.g., the other companies and individuals that use Copywrite outside of HashiCorp).

@ghost ghost requested review from mukeshjc and CalebAlbers February 13, 2025 05:55
@@ -0,0 +1,11 @@
/* This policy requires that the `require_lowercase_characters` attribute of the `aws_iam_account_password_policy`

Choose a reason for hiding this comment

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

@sonamtenzin6 also add a test case where there is no policy description mentioned.

Copy link
Author

Choose a reason for hiding this comment

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

sure, added under file4.sentinel

Choose a reason for hiding this comment

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

Thanks, Also could you please rename the files to the scenario that we are testing. e.g. single_line_description.sentinel

@ghost ghost requested a review from mallikabandaru February 13, 2025 07:22
@@ -0,0 +1,11 @@
/* This policy requires that the `require_lowercase_characters` attribute of the `aws_iam_account_password_policy`

Choose a reason for hiding this comment

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

Thanks, Also could you please rename the files to the scenario that we are testing. e.g. single_line_description.sentinel

@mukeshjc
Copy link
Contributor

Suggestion: Can we execute the tool locally on one of the policy library repos (link)? So that we can ensure this will work as expected after the change is merged.

@ghost
Copy link
Author

ghost commented Feb 13, 2025

Suggestion: Can we execute the tool locally on one of the policy library repos (link)? So that we can ensure this will work as expected after the change is merged.

Did a dry run on the mentioned PR after removing existing headers from the file, the new header was always getting added after any comment on the top for sentinel files

@ghost ghost requested review from mallikabandaru and mukeshjc February 13, 2025 09:57
mukeshjc
mukeshjc previously approved these changes Feb 19, 2025
Copy link
Contributor

@mukeshjc mukeshjc left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for fixing this issue.

@mukeshjc mukeshjc removed the request for review from CalebAlbers February 19, 2025 05:17
@mallikabandaru
Copy link

LGTM. Thanks for incorporating all the comments.

mallikabandaru
mallikabandaru previously approved these changes Feb 19, 2025
@ghost ghost dismissed stale reviews from mallikabandaru and mukeshjc via 3719a22 February 20, 2025 05:33
@ghost ghost requested a review from mukeshjc February 20, 2025 05:44
mukeshjc
mukeshjc previously approved these changes Feb 20, 2025
@ghost ghost merged commit 9d021bf into main Feb 20, 2025
5 checks passed
@ghost ghost deleted the sonamt/sentinel-fix branch February 20, 2025 05:53
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants