Skip to content
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

Handled sentinel policy scenario for copywrite header #119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sonamtenzin6
Copy link

🛠️ 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?

@sonamtenzin6
Copy link
Author

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

@sonamtenzin6 sonamtenzin6 marked this pull request as ready for review February 11, 2025 06:43
@sonamtenzin6 sonamtenzin6 requested a review from a team 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).

@@ -414,8 +414,31 @@ var head = []string{
"/** @jest-environment", // Jest Environment string https://jestjs.io/docs/configuration#testenvironment-string
}

var headPatterns = []string{
`# This policy requires.*\s*(#.*\s*)*`,
Copy link
Member

Choose a reason for hiding this comment

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

Do Sentinel policies themselves enforce comments at the top of strings that use this format?

I could be wrong, but it feels like this route may not be robust in the face of sentinel policies that start with other verbiage. A contrived example would be one that starts with:

# These policies require...

Here is an example of a Sentinel policy that starts with a different top line: https://github.com/hashicorp/vault-guides/blob/master/governance/sentinel/cidr-policy.sentinel

A cursory search on GitHub shows that most people use the .sentinel extension. Could that be a viable alternative to match on instead?

Copy link
Contributor

@mukeshjc mukeshjc Feb 11, 2025

Choose a reason for hiding this comment

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

@CalebAlbers

TLDR
your assessment is accurate. I have synced with @sonamtenzin6 to publish an updated PR that will handle our requirements in a better way for all ".sentinel" files.

Long form
HashiCorp's pre-written sentinel policy libraries conventionally use top file comments for policy descriptions, which TFC UI displays. The current copyright headers disrupt this by appearing first, causing TFC UI to display them instead of policy descriptions.
To address this urgently, we will:

  1. Add support for .sentinel files in copywrite tool
  2. Place copyright headers after top comments in sentinel policies to maintain TFC UI functionality

This workaround accommodates TFC's current comment parsing behavior while preserving both policy descriptions and copyright information.

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.

3 participants