-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: Update Hub and ACM submodules to use native Terraform resources #947
Conversation
985585c
to
e03fe58
Compare
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days |
04ed44c
to
4fa1e28
Compare
4ba1c7c
to
ccb2b4b
Compare
@bharathkkb This should be ready to review. |
2d9021c
to
5875c6b
Compare
5875c6b
to
9488583
Compare
Thanks for the PR! 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
modules/hub-acm-feature/main.tf
Outdated
project = var.project_id | ||
|
||
configmanagement { | ||
version = "1.11.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't this result in possibly older versions being installed? We could default to API with a user override for explicit version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's preferable to be explicit with pinning a version here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should expose this as a var defaulting to 1.11.0
? I assume users will want to use other versions and it can be toilsome to keep bumping this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd still have to keep bumping the default though. A new variable can always be added in a later (minor) release, but for now I think hard-coding is sensible to keep the API surface small.
9a41798
to
2a82f24
Compare
388d7ab
to
2d762a5
Compare
@bharathkkb Ready for another review |
9d8912a
to
e577ceb
Compare
Fixes #945.
Note: for now I have left the existing
hub
module alone so it can be used to register non-GKE clusters.Unfortunately Hub is not presently able to push out the SSH key to clusters so we still need to have separate resources for that. I'm exploring using the
kubernetes
provider to do this.