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(acm)!: remove direct kubectl commands #1751

Merged
merged 2 commits into from
Oct 23, 2023

Conversation

foosinn
Copy link
Contributor

@foosinn foosinn commented Sep 28, 2023

kubectl-wrapper currently breaks if one has to access the api using a proxy (IAP).

This replaces the wrapper call for all annotations with the kubernetes_annotations resource. The kubernetes provider is already in use for the secret.

In addition it allows the user to disable the automated restart. Sadly it's currently not possible to work around this AFAIK

@foosinn foosinn requested review from Jberlinsky, ericyz and a team as code owners September 28, 2023 16:47
@google-cla
Copy link

google-cla bot commented Sep 28, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@foosinn foosinn force-pushed the nokubectl branch 7 times, most recently from a9ebe65 to 7d5653f Compare September 29, 2023 17:11
@foosinn
Copy link
Contributor Author

foosinn commented Sep 29, 2023

fixed the docs

@apeabody
Copy link
Contributor

/gcbrun

@foosinn
Copy link
Contributor Author

foosinn commented Oct 5, 2023

@apeabody any update on this? would love to switch back to upstream :)

@apeabody
Copy link
Contributor

apeabody commented Oct 5, 2023

/gcbrun

@foosinn
Copy link
Contributor Author

foosinn commented Oct 5, 2023

@apeabody I've got some feedback on the kubernetes restart handler:

hashicorp/terraform-provider-kubernetes#2304 (comment)

Would you prefer to leave it as is, or should i update the PR for the recommended solution? I'm not really sure of the use of time_static would have any implications to your release cycle

@apeabody
Copy link
Contributor

apeabody commented Oct 5, 2023

@apeabody I've got some feedback on the kubernetes restart handler:

hashicorp/terraform-provider-kubernetes#2304 (comment)

Would you prefer to leave it as is, or should i update the PR for the recommended solution? I'm not really sure of the use of time_static would have any implications to your release cycle

Hi @foosinn - This will be going into major version (#1746), so I would recommend including everything in this PR so any upgrade migration required is only one-time. It appears we already include the time provider as a requirement. Any required upgrade documentation can be added as docs/upgrading_to_v29.0.md

@foosinn
Copy link
Contributor Author

foosinn commented Oct 5, 2023

i'll update tomorrow to the recommended fix.

Can you somehow share the log output of Build 5d254629-88a8-47ca-bc26-da74d399ea9f failed? Seems like it's not public

@apeabody
Copy link
Contributor

apeabody commented Oct 5, 2023

i'll update tomorrow to the recommended fix.

Can you somehow share the log output of Build 5d254629-88a8-47ca-bc26-da74d399ea9f failed? Seems like it's not public

Failure appears unrelated (timeout), I've re-triggered the run.

@foosinn foosinn force-pushed the nokubectl branch 2 times, most recently from 7a3a998 to 2c47ab6 Compare October 6, 2023 12:28
@foosinn
Copy link
Contributor Author

foosinn commented Oct 6, 2023

Switched to the suggested solution and also rebased latest master. Let me know if you need anything else.

Did also a quick test with our fork, seems to work fine :)

@apeabody
Copy link
Contributor

apeabody commented Oct 6, 2023

From the linter

Checking submodule's files generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=.git' /workspace/modules/acm/README.md /tmp/tmp.vcv4AgP5L4/workspace/modules/acm/README.md
98d97
< | restart\_gatekeeper\_controller\_manager | Restart the gatekeeper controller manager after setting up workload id (needs to be done manually if a proxy to gke api is required) | `bool` | `true` | no |
Error: submodule's files generation has not been run, please run the
'make build' command and commit changes
/usr/local/bin/task_helper_functions.sh: line 30: DELETE_AT_EXIT: readonly variable
Checking for documentation generation
diff -r '--exclude=.terraform' '--exclude=.kitchen' '--exclude=autogen' '--exclude=*.tfvars' '--exclude=*metadata.yaml' /workspace/modules/acm/README.md /tmp/tmp.ONYkjFiaEt/generate_docs/workspace/modules/acm/README.md
98d97
< | restart\_gatekeeper\_controller\_manager | Restart the gatekeeper controller manager after setting up workload id (needs to be done manually if a proxy to gke api is required) | `bool` | `true` | no |
Error: Documentation generation has not been run, please run the
'make docker_generate_docs' command and commit the above changes.

@foosinn
Copy link
Contributor Author

foosinn commented Oct 9, 2023

And again fixed the docs

@apeabody
Copy link
Contributor

apeabody commented Oct 9, 2023

/gcbrun

kubectl-wrapper currently breaks if one has to access the api using a
proxy (IAP).
@apeabody
Copy link
Contributor

/gcbrun

@foosinn
Copy link
Contributor Author

foosinn commented Oct 11, 2023

Can you share the error that happened?

@apeabody apeabody changed the title feat(acm): remove direct kubectl commands feat(acm)!: remove direct kubectl commands Oct 23, 2023
Copy link
Contributor

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @foosinn

Marking as a breaking change given the config "churn" window.

@apeabody
Copy link
Contributor

/gcbrun

@apeabody apeabody merged commit 4c27a6a into terraform-google-modules:master Oct 23, 2023
4 checks passed
@foosinn
Copy link
Contributor Author

foosinn commented Oct 24, 2023

thanks!

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

2 participants