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

chore: Adjusts check-changelog-entry-file to verify the entry types our guidelines define #2179

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

AgustinBettati
Copy link
Member

@AgustinBettati AgustinBettati commented Apr 19, 2024

Description

Link to any related issue(s): CLOUDP-244157

Our check-changelog-entry-file script was currently delegating the validation logic completely to go-changelog entry.Validate(), but this does have to downside that its allowed types are hardcoded and not configurable (hashicorp/go-changelog#6). The hardcoded values are not fully aligned with our allowed entry types.

This PR extracts the validation logic and ensures that we verify our allowed entry types.

Testing

  • Using an unknown type: 2024/04/19 12:50:26 Error validating changelog file: .changelog/3000.txt. Unknown changelog types [other], please use only the configured changelog entry types [breaking-change new-resource new-datasource new-guide note enhancement bug]
  • Using new-guide type (this was throwing an error before the changes): Changelog entry file is valid: .changelog/3000.txt

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contributing guides
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.
  • If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

@AgustinBettati AgustinBettati marked this pull request as ready for review April 19, 2024 12:45
@AgustinBettati AgustinBettati requested a review from a team as a code owner April 19, 2024 12:45
@@ -76,3 +100,21 @@ func skipLabel(labels []string) bool {
}
return false
}

func getValidTypes(path string) []string {
file, err := os.Open(path)
Copy link
Member

@lantoli lantoli Apr 19, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@lantoli lantoli Apr 19, 2024

Choose a reason for hiding this comment

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

and I think later you can get the lines, e.g. with: lines := strings.Split(content, "\n")

Copy link
Member Author

@AgustinBettati AgustinBettati Apr 19, 2024

Choose a reason for hiding this comment

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

thanks for the suggestion, simplified the function 👍

fmt.Printf("Changelog entry file is valid: %s\n", filePath)
}

func isValidType(t string) bool {
Copy link
Member

@lantoli lantoli Apr 19, 2024

Choose a reason for hiding this comment

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

knit: a bit confusing this short variable names but i guess you got them as it is

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to make more clear

skipTitles = []string{"chore", "test", "doc", "ci"} // Dependabot uses chore.
skipLabelName = "skip-changelog-check"
skipTitles = []string{"chore", "test", "doc", "ci"} // Dependabot uses chore.
allowedTypeValues = getValidTypes("scripts/changelog/allowed-types.txt")
Copy link
Member

Choose a reason for hiding this comment

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

nice to have the values in a common place

@@ -53,12 +55,34 @@ func validateChangelog(filePath, body string) {
entry := changelog.Entry{
Body: body,
}
if err := entry.Validate(); err != nil {
log.Fatalf("Error validating changelog file: %s, err: %v", filePath, err)
notes := changelog.NotesFromEntry(entry)
Copy link
Member

Choose a reason for hiding this comment

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

knit: it might be worth it to have a comment pointing to the original code so we don't forget to check from time to time to see if they made it more extensible and can use it

Copy link
Member Author

Choose a reason for hiding this comment

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

added a small comment giving this context

@oarbusi
Copy link
Collaborator

oarbusi commented Apr 19, 2024

Our allowed entry types a subset of the hardcoded go-changelog entry.Validate() types. Do we expect our allowed entry types to be different than those? (more entry types than the ones hardcoded)
edit: sorry just saw the new-guide case

@AgustinBettati AgustinBettati merged commit b8fa30d into master Apr 19, 2024
31 checks passed
@AgustinBettati AgustinBettati deleted the CLOUDP-244157 branch April 19, 2024 15:02
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.

None yet

3 participants