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

feat: Slugified the branch name in PR generators #9462

Merged
merged 5 commits into from Jun 8, 2022

Conversation

Aym3nTN
Copy link
Contributor

@Aym3nTN Aym3nTN commented May 20, 2022

Enhancement proposal/Fix: #9461

Fixes #9461

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? Yes, WIP
  • 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).

@Aym3nTN Aym3nTN changed the title Slugified the branch name in PR generators feat: Slugified the branch name in PR generators May 20, 2022
@Aym3nTN Aym3nTN force-pushed the slugified-pr-branch-name branch 2 times, most recently from 527fb03 to 46ca395 Compare May 20, 2022 13:37
@codecov
Copy link

codecov bot commented May 20, 2022

Codecov Report

Merging #9462 (33b6f3e) into master (8c96a36) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9462      +/-   ##
==========================================
- Coverage   45.86%   45.84%   -0.03%     
==========================================
  Files         221      222       +1     
  Lines       26309    26409     +100     
==========================================
+ Hits        12067    12106      +39     
- Misses      12586    12652      +66     
+ Partials     1656     1651       -5     
Impacted Files Coverage Δ
...is/applicationset/v1alpha1/applicationset_types.go 34.69% <ø> (+2.55%) ⬆️
applicationset/generators/pull_request.go 42.46% <100.00%> (+3.33%) ⬆️
util/argo/managedfields/managed_fields.go 42.04% <0.00%> (-32.96%) ⬇️
util/helm/helm.go 41.89% <0.00%> (-19.45%) ⬇️
server/application/terminal.go 12.83% <0.00%> (-11.49%) ⬇️
...licationset/generators/generator_spec_processor.go 51.66% <0.00%> (-5.91%) ⬇️
util/helm/cmd.go 25.29% <0.00%> (-3.53%) ⬇️
controller/state.go 73.64% <0.00%> (-0.26%) ⬇️
controller/appcontroller.go 52.07% <0.00%> (-0.21%) ⬇️
controller/sync.go 56.84% <0.00%> (-0.12%) ⬇️
... and 15 more

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 8c96a36...33b6f3e. Read the comment docs.

@Aym3nTN Aym3nTN force-pushed the slugified-pr-branch-name branch 3 times, most recently from 87a5d95 to c7883fe Compare May 20, 2022 17:47
@crenshaw-dev
Copy link
Collaborator

@Aym3nTN thanks for the PR! Instead of changing the branch variable, can you introduce a new branchSlug field? I'd rather this not be a breaking change for users of the original field.

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.

+branchSlug

@Aym3nTN
Copy link
Contributor Author

Aym3nTN commented May 29, 2022

I agree with you.. I'd suggest also making branchSlug field boolean defaulting to false to not cause a breaking change what do you think?

@Aym3nTN Aym3nTN force-pushed the slugified-pr-branch-name branch 5 times, most recently from e3734d8 to 4e9c942 Compare May 31, 2022 08:25
@crenshaw-dev
Copy link
Collaborator

@Aym3nTN instead of introducing a new config field that toggles the branch contents, how would you feel about calculating both and just dropping the slugified version into a new branchSlug param? That would be helpful if someone wants both versions.

Aymen Ben Tanfous added 3 commits June 8, 2022 10:24
Add branchSlug attr to not cause a breaking change

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
@Aym3nTN
Copy link
Contributor Author

Aym3nTN commented Jun 8, 2022

@crenshaw-dev I totally agree with you on; and also it's less risky that way.. keeping the old branch value for those who already using it will avoid us from having a breaking change.

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
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.

Thanks @Aym3nTN!

@crenshaw-dev crenshaw-dev merged commit f209ae1 into argoproj:master Jun 8, 2022
Aym3nTN added a commit to Aym3nTN/argo-cd that referenced this pull request Sep 15, 2022
* Slugified the branch name in PR generators
Add branchSlug attr to not cause a breaking change

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>

* Fixed pull_request_test

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>

* Adding 'branch_slug' as an output of the generator

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>

* Updated the doc

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>

* Fixed test

Signed-off-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>

Co-authored-by: Aymen Ben Tanfous <aymen.bentanfous@cimpress.com>
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.

Cleanup branch name in PR Generators
2 participants