-
Notifications
You must be signed in to change notification settings - Fork 19
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
Conversation
Able to build the branch on local, the |
There was a problem hiding this 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).
@@ -0,0 +1,11 @@ | |||
/* This policy requires that the `require_lowercase_characters` attribute of the `aws_iam_account_password_policy` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
@@ -0,0 +1,11 @@ | |||
/* This policy requires that the `require_lowercase_characters` attribute of the `aws_iam_account_password_policy` |
There was a problem hiding this comment.
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
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 |
There was a problem hiding this 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.
LGTM. Thanks for incorporating all the comments. |
🛠️ 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
🤔 Can be merged upon approval?
✅