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

Automatically fall back to client-side preview if server-side preview fails #2419

Merged
merged 4 commits into from
May 23, 2023

Conversation

lblackstone
Copy link
Member

@lblackstone lblackstone commented May 18, 2023

Proposed changes

Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.

Related issues (optional)

Fix #2411

@github-actions
Copy link

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.

… fails

Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.
@lblackstone lblackstone changed the title Automatically fall back to CSA mode for preview if SSA fails Automatically fall back to client-side preview if server-side preview fails May 19, 2023
@lblackstone lblackstone marked this pull request as ready for review May 22, 2023 20:53
@lblackstone
Copy link
Member Author

lblackstone commented May 22, 2023

Without the provider change, the test fails with the following errors:

error: namespaces "test-30a036a4" is forbidden: User "system:serviceaccount:kube-system:kubeconfig-sa" cannot patch resource "namespaces" in API group "" in the namespace "test-30a036a4"
error: configmaps "foo" is forbidden: User "system:serviceaccount:kube-system:kubeconfig-sa" cannot delete resource "configmaps" in API group "" in the namespace "test-30a036a4"

Copy link
Contributor

@rquitales rquitales left a comment

Choose a reason for hiding this comment

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

Mostly LGTM! Just 1 comment to address.

tests/sdk/nodejs/nodejs_test.go Outdated Show resolved Hide resolved
@lblackstone lblackstone merged commit 7a55945 into v4 May 23, 2023
18 checks passed
@lblackstone lblackstone deleted the lblackstone/csa-fallback branch May 23, 2023 19:42
lblackstone added a commit that referenced this pull request Jul 14, 2023
… fails (#2419)

Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.
lblackstone added a commit that referenced this pull request Jul 14, 2023
… fails (#2419)

Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.
lblackstone added a commit that referenced this pull request Jul 17, 2023
… fails (#2419)

Server-side apply previews currently require "patch" permission to run. For cases where the user doesn't have permission to perform a "patch" operation, attempt a graceful fallback to Client-side preview. The Client-side preview may not be 100% accurate, but is preferable to failing with a permission error.
@sam-utila
Copy link

Great job, just needed it

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

4 participants