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.

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

feat(applicationset): add short sha to PR generator #9668 #9669

Merged
merged 2 commits into from Jun 29, 2022
Merged

feat(applicationset): add short sha to PR generator #9668 #9669

merged 2 commits into from Jun 29, 2022

Conversation

Redlinkk
Copy link
Contributor

@Redlinkk Redlinkk commented Jun 15, 2022 •
edited

Fixes #9668

Signed-off-by: Romain Poirot github@romainpoirot.fr

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@codecov
Copy link

codecov bot commented Jun 15, 2022 •
edited

Codecov Report

Merging #9669 (5045bce) into master (b524769) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9669      +/-   ##
==========================================
+ Coverage   45.84%   45.86%   +0.02%     
==========================================
  Files         227      227              
  Lines       26846    26856      +10     
==========================================
+ Hits        12307    12317      +10     
  Misses      12859    12859              
  Partials     1680     1680
Impacted Files Coverage Δ
applicationset/generators/pull_request.go 46.15% <100.00%> (+3.68%) ⬆️
applicationset/generators/scm_provider.go 41.11% <100.00%> (+3.46%) ⬆️
util/settings/settings.go 48.16% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b524769...5045bce. Read the comment docs.

rishabh625
rishabh625 approved these changes Jun 15, 2022
View reviewed changes
Copy link
Contributor

@rishabh625 rishabh625 left a comment

Choose a reason for hiding this comment

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

LGTM

ishitasequeira
ishitasequeira approved these changes Jun 15, 2022
View reviewed changes
Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

Thanks @Redlinkk for the PR.
LGTM!

@Redlinkk Redlinkk force-pushed the add_short_sha_to_pr_generator branch 2 times, most recently from 82d13fa to 8133222 Compare June 16, 2022 16:16
@Redlinkk
Copy link
Contributor Author

I've also added the short sha to the SCM generator as @nar3k requested it on the issue

@Redlinkk Redlinkk force-pushed the add_short_sha_to_pr_generator branch from 8133222 to d0ddb7e Compare June 17, 2022 12:17
@benkn
Copy link
Contributor

benkn commented Jun 17, 2022

Could it be 8 characters? For Gitlab, it's short uses 8 characters -- CI_COMMIT_SHORT_SHA in https://docs.gitlab.com/ee/ci/variables/predefined_variables.html

@Redlinkk Redlinkk force-pushed the add_short_sha_to_pr_generator branch 2 times, most recently from 4d114a9 to 7a4aca7 Compare June 20, 2022 07:53
@Redlinkk
Copy link
Contributor Author

You are absolutely right @benkn, I made a mistake in my spec document, and I blindly followed it. It should now be fixed. Thanks for the review.

crenshaw-dev
crenshaw-dev requested changes Jun 28, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

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

One nitpick.

applicationset/generators/pull_request.go Outdated
"branch": pull.Branch,
"branch_slug": slug.Make(pull.Branch),
"head_sha": pull.HeadSHA,
"head_short_sha": pull.HeadSHA[:8],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a length check, just in case the API returns an empty (or short) string?

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 added the length check. If the head_sha is shorter than 8 characters, then I return the same value for the head_short_sha.

applicationset/generators/scm_provider.go Outdated
@@ -127,6 +127,7 @@ func (g *SCMProviderGenerator) GenerateParams(appSetGenerator *argoprojiov1alpha
"url": repo.URL,
"branch": repo.Branch,
"sha": repo.SHA,
"short_sha": repo.SHA[:8],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should there be a length check, just in case the API returns an empty (or short) string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above

@Redlinkk Redlinkk force-pushed the add_short_sha_to_pr_generator branch from 7a4aca7 to f1ea9f3 Compare June 29, 2022 12:56
@Redlinkk
feat(applicationset): add short sha to PR generator
9791e81 
Signed-off-by: Romain Poirot <github@romainpoirot.fr>
@Redlinkk
feat(applicationset): add short sha to SCM generator
5045bce 
Signed-off-by: Romain Poirot <github@romainpoirot.fr>
@Redlinkk Redlinkk force-pushed the add_short_sha_to_pr_generator branch from f1ea9f3 to 5045bce Compare June 29, 2022 12:57
crenshaw-dev
crenshaw-dev approved these changes Jun 29, 2022
View reviewed changes
Copy link
Collaborator

@crenshaw-dev crenshaw-dev 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!

@crenshaw-dev crenshaw-dev merged commit 2ac7bd3 into argoproj:master Jun 29, 2022
@fty4 fty4 mentioned this pull request Jan 13, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crenshaw-dev crenshaw-dev crenshaw-dev approved these changes

@rishabh625 rishabh625 rishabh625 approved these changes

@ishitasequeira ishitasequeira ishitasequeira approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add short SHA in PR generator
5 participants
@Redlinkk @benkn @crenshaw-dev @rishabh625 @ishitasequeira