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

Upgrade to latest helm dependency #2474

Merged
merged 2 commits into from
Jun 29, 2023
Merged

Upgrade to latest helm dependency #2474

merged 2 commits into from
Jun 29, 2023

Conversation

lblackstone
Copy link
Member

Proposed changes

Related issues (optional)

@lblackstone lblackstone requested a review from a team June 27, 2023 18:30
thomas11
thomas11 previously approved these changes Jun 27, 2023
@github-actions
Copy link

github-actions bot commented Jun 27, 2023

Does the PR have any schema changes?

Found 2 breaking changes:
Resource "kubernetes:helm.sh/v2:Chart" missing
Type "kubernetes:helm.sh/v2:FetchOpts" missing
No new resources/functions.

@lblackstone lblackstone enabled auto-merge (squash) June 27, 2023 19:06
@lblackstone lblackstone enabled auto-merge (squash) June 27, 2023 21:18
@lblackstone lblackstone force-pushed the lblackstone/update-helm branch 3 times, most recently from 590c4c3 to f10e7ca Compare June 28, 2023 16:15
@rquitales
Copy link
Contributor

rquitales commented Jun 28, 2023

Just added a commit to bump k8s to v1.27 for the test cluster. It seems like the client libraries being upgraded here is causing some diffing issues when the test cluster used is 1.24.13-gke.2500 resulting in the failing test. I haven't done any additional tests to see if it passes on v1.25, or v1.26 either, but GKE v1.27 works. I have also not tested if this is an issue with OSS Kubernetes, or GKE.

rquitales
rquitales previously approved these changes Jun 28, 2023
@rquitales
Copy link
Contributor

Did more testing, and the test fails on OSS Kubernetes (KinD) and not just GKE.

Tested:

  • v1.23.17 (EOL 2023-02-28): FAIL
  • v1.24.15 (Latest v1.24.x): FAIL
  • v1.24.13: FAIL
  • v1.24.1: FAIL

  • v1.25.11: PASS
  • v1.26.6: PASS
  • v1.27.3: PASS

Looks like this upgrade is causing a regression for all clusters with v1.24 or lower. @lblackstone What do you think about landing this change?

@rquitales rquitales dismissed their stale review June 28, 2023 20:54

This PR causes a regression in older clusters.

@lblackstone lblackstone marked this pull request as draft June 28, 2023 22:26
auto-merge was automatically disabled June 28, 2023 22:26

Pull request was converted to draft

@lblackstone
Copy link
Member Author

What do you think about landing this change?

Lets hold off for now. This change isn't required at this time, so let's figure out what's causing the regression and we can ideally wait until it's fixed upstream.

@lblackstone
Copy link
Member Author

I believe that I identified and fixed the problem causing the test failures. The updated client-go dependency no longer includes dry-run verifier support, and my initial change erroneously changed the support check to use the wrong verifier flag. As a result, the supportsDryRun function always returned false, so the Diff used inputs rather than the Server-side dry run. There is a known bug with input diffing that fails to detect drift, so the test was failing for this reason.

I updated the dry-run support check to require cluster version of v1.13+ rather than querying the cluster, and verified that the test passes again.

@lblackstone lblackstone marked this pull request as ready for review June 29, 2023 00:26
@rquitales rquitales dismissed thomas11’s stale review June 29, 2023 00:40

This was reviewed prior to latest changes that addresses the dry-run logic change.

@lblackstone lblackstone changed the base branch from master to v4 June 29, 2023 00:48
@lblackstone lblackstone marked this pull request as draft June 29, 2023 00:48
@lblackstone
Copy link
Member Author

We decided to move this change to the v4 branch, which already drops support for clusters older than v1.13. I'll rework the PR for that.

@lblackstone lblackstone marked this pull request as ready for review June 29, 2023 04:27
@lblackstone
Copy link
Member Author

@rquitales Mind giving this another look?

@lblackstone lblackstone merged commit f1e790b into v4 Jun 29, 2023
18 checks passed
@lblackstone lblackstone deleted the lblackstone/update-helm branch June 29, 2023 19:28
lblackstone added a commit that referenced this pull request Jul 14, 2023
* Upgrade to latest helm dependency

* Remove dry-run verifier code
lblackstone added a commit that referenced this pull request Jul 14, 2023
* Upgrade to latest helm dependency

* Remove dry-run verifier code
lblackstone added a commit that referenced this pull request Jul 17, 2023
* Upgrade to latest helm dependency

* Remove dry-run verifier code
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