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

chore!: Helm2 removal #8313

Merged
merged 3 commits into from
Apr 9, 2022
Merged

chore!: Helm2 removal #8313

merged 3 commits into from
Apr 9, 2022

Conversation

shuker85
Copy link
Contributor

@shuker85 shuker85 commented Jan 28, 2022 •
edited by crenshaw-dev
Loading

Fixes: #8228

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).

Closes [#8228]

Also updated CRD test and structure since the old beta api is deprecated.

@codecov
Copy link

codecov bot commented Jan 28, 2022 •
edited
Loading

Codecov Report

Merging #8313 (a80eff9) into master (1a47fa2) will decrease coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8313      +/-   ##
==========================================
- Coverage   45.23%   45.17%   -0.07%     
==========================================
  Files         214      214              
  Lines       25438    25436       -2     
==========================================
- Hits        11508    11491      -17     
- Misses      12310    12327      +17     
+ Partials     1620     1618       -2
Impacted Files Coverage Δ
pkg/apis/application/v1alpha1/types.go 54.63% <ø> (ø)
util/helm/cmd.go 28.82% <ø> (-0.83%) ⬇️
util/helm/helmver.go 80.00% <ø> (ø)
applicationset/services/scm_provider/github.go 63.52% <0.00%> (-17.65%) ⬇️
reposerver/repository/repository.go 59.55% <0.00%> (-0.43%) ⬇️
applicationset/services/scm_provider/utils.go 88.50% <0.00%> (+4.59%) ⬆️

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 1a47fa2...a80eff9. Read the comment docs.

crenshaw-dev
crenshaw-dev requested changes Jan 31, 2022
View reviewed changes
test/container/Dockerfile Outdated Show resolved Hide resolved
test/container/Dockerfile Show resolved Hide resolved
test/remote/run-e2e-remote.sh Outdated Show resolved Hide resolved
util/helm/helm_test.go Outdated Show resolved Hide resolved
@shuker85 shuker85 requested a review from crenshaw-dev February 1, 2022 09:56
crenshaw-dev
crenshaw-dev requested changes Feb 2, 2022
View reviewed changes
Copy link
Member

@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.

Some of the deleted e2e tests look like them might have coincidentally referenced some helm2 test data, but the tests might still apply to helm3. Gonna look a bit more tomorrow.

@shuker85 shuker85 requested a review from crenshaw-dev February 2, 2022 15:17
crenshaw-dev
crenshaw-dev requested changes Feb 2, 2022
View reviewed changes
reposerver/repository/repository_test.go Show resolved Hide resolved
reposerver/repository/repository_test.go Show resolved Hide resolved
util/helm/helm_test.go Outdated Show resolved Hide resolved
util/helm/helm_test.go Outdated Show resolved Hide resolved
@shuker85 shuker85 requested a review from crenshaw-dev February 3, 2022 11:21
crenshaw-dev
crenshaw-dev requested changes Feb 3, 2022
View reviewed changes
Copy link
Member

@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 more small question. Thanks for your persistence, I think it's 99.9% there!

reposerver/repository/repository_norace_test.go Show resolved Hide resolved
crenshaw-dev
crenshaw-dev approved these changes Feb 3, 2022
View reviewed changes
Copy link
Member

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

Just a note: there's a reference in a comment about Helm version 2 vs. 3. That comment ends up in the swagger doc (and maybe the CRD validation info?). Should we remove the "2"?

argo-cd/pkg/apis/application/v1alpha1/types.go

Line 247 in f391256

// Version is the Helm version to use for templating (either "2" or "3")

@shuker85
Copy link
Contributor Author

shuker85 commented Feb 3, 2022

@crenshaw-dev any hints how the swagger stuff should be fixed ?

@crenshaw-dev
Copy link
Member

@shuker85 I think you want to make openapigen. It's a subset of make codegen.

@shuker85 shuker85 force-pushed the helm2-removal branch 4 times, most recently from d4bafb3 to 8484484 Compare February 7, 2022 20:12
@shuker85 shuker85 requested a review from crenshaw-dev February 7, 2022 21:02
@shuker85 shuker85 force-pushed the helm2-removal branch 2 times, most recently from 753c33c to 8d14cd2 Compare February 8, 2022 14:25
@rbreeze
Copy link
Member

rbreeze commented Mar 1, 2022

@shuker85 Can you resolve merge conflicts? Otherwise LGTM

@shuker85 shuker85 force-pushed the helm2-removal branch 2 times, most recently from 3acfa21 to 5ff47b0 Compare March 2, 2022 20:32
shuker85 and others added 2 commits March 27, 2022 18:19
@shuker85
chore: remove helm2
269cba9 
Reorder test/container/Dockerfile to mitigate issue of being unable to create
.gitconfig since the homedir is not present

chore: cleanup helm2 and tests related to it
Remove helm2 init. Fix unused import
Use helm 3 structure for CRDs
Remove helm2-dependency testdata
Address PR comments
Add back values-production and value.yaml on helm tests
Remove helm2 from openapi.

Signed-off-by: Shyukri Shyukriev <shyukri.shyukriev@mariadb.com>

	modified:   util/helm/cmd_test.go
@crenshaw-dev @shuker85
fix: generated openapi
44e648f 
Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>
ishitasequeira
ishitasequeira approved these changes Mar 31, 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.

@crenshaw-dev I think this is ready to be merged

@crenshaw-dev
Copy link
Member

Nice! @alexmt or @jannfis do we need to make a note anywhere that this is a breaking change so people upgrading to 2.4 aren't caught off guard?

@crenshaw-dev crenshaw-dev changed the title chore: Helm2 removal chore!: Helm2 removal Apr 9, 2022
@crenshaw-dev crenshaw-dev added the breaking/medium A possibly breaking change with medium impact label Apr 9, 2022
@crenshaw-dev crenshaw-dev enabled auto-merge (squash) April 9, 2022 18:28
@crenshaw-dev crenshaw-dev merged commit 6b783af into argoproj:master Apr 9, 2022
wojtekidd pushed a commit to wojtekidd/argo-cd that referenced this pull request Apr 25, 2022
@shuker85 @crenshaw-dev @wojtekidd
chore!: Helm2 removal (argoproj#8313)
1d8b420 
* chore: remove helm2

Reorder test/container/Dockerfile to mitigate issue of being unable to create
.gitconfig since the homedir is not present

chore: cleanup helm2 and tests related to it
Remove helm2 init. Fix unused import
Use helm 3 structure for CRDs
Remove helm2-dependency testdata
Address PR comments
Add back values-production and value.yaml on helm tests
Remove helm2 from openapi.

Signed-off-by: Shyukri Shyukriev <shyukri.shyukriev@mariadb.com>

	modified:   util/helm/cmd_test.go

* fix: generated openapi

Signed-off-by: Michael Crenshaw <michael@crenshaw.dev>

Co-authored-by: Michael Crenshaw <michael@crenshaw.dev>
Signed-off-by: wojtekidd <wojtek.cichon@protonmail.com>
@SharifiFaranak
Copy link

@crenshaw-dev Noticed this change after upgrading to Argo and seeing rpc error: code = Unknown desc = Manifest generation error (cached): helm chart version 'v2' is not supported
I think this could be considered a breaking change.
Also docs still mention that Helm v2 can be used.
Thanks!

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

@ishitasequeira ishitasequeira ishitasequeira approved these changes

Assignees
No one assigned
Labels
breaking/medium A possibly breaking change with medium impact
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Stop bundling and shipping Helm v2 binaries
5 participants
@shuker85 @crenshaw-dev @rbreeze @SharifiFaranak @ishitasequeira