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

Fix pinning of machine_type #149

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Fix pinning of machine_type #149

wants to merge 22 commits into from

Conversation

dimisjim
Copy link

what

Achieve pinning of COS image by fixing behavior of machine_image var and input to downstream container module

why

  • Be able to pin image and not always use latest one

references

@dimisjim dimisjim requested a review from a team as a code owner April 18, 2024 09:17
@dimisjim dimisjim mentioned this pull request Apr 18, 2024
Open
@dimisjim
Copy link
Author 

remote: Permission to dimisjim/terraform-gce-atlantis.git denied to github-actions[bot].
fatal: unable to access 'https://github.com/dimisjim/terraform-gce-atlantis/': The requested URL returned error: 403

not sure what it causing exactly the error in the CI run but I have just enabled workflows in the forked repo, so maybe you can try running it again 👍

@d-costa
Copy link
Collaborator

d-costa commented Apr 19, 2024 •
edited
Loading

@dimisjim that error is due to the terraform-docs trying to fixing the docs, and fails to commit - which makes sense
we should probably disable the git-push flag when the workflow runs on forks

#150 should fix this issue

@d-costa d-costa mentioned this pull request Apr 19, 2024
Merged
@d-costa
Copy link
Collaborator

d-costa commented Apr 19, 2024 •
edited
Loading

@dimisjim, this docker oneliner should inject the fix locally:

docker run --rm --volume "$(pwd):/terraform-docs" -u $(id -u) quay.io/terraform-docs/terraform-docs:0.17.0 markdown --output-file README.md --output-mode inject /terraform-docs

Could you please run this, rebase and squash the commits?

Source: https://github.com/terraform-docs/terraform-docs?tab=readme-ov-file#using-docker

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Apr 20, 2024
@dimisjim
add machine_type example
04372a2 
add cos_image_name to container module, by only using the last element of the string

tf fmt

tf fmt fix

ran terraform-docs
@dimisjim
Copy link
Author

cool, should be done 👍

bschaatsbergen
bschaatsbergen requested changes Apr 21, 2024
View reviewed changes
Copy link
Member

@bschaatsbergen bschaatsbergen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey @dimisjim, thanks for taking the time to submit this pull request - good catch! 👏🏼

What would occur if a user doesn't specify the machine_image variable? Given that the default is null, I anticipate the following error: Invalid value for "str" parameter: argument must not be null. Please correct me if I'm mistaken.

@dimisjim
Copy link
Author

dimisjim commented Apr 21, 2024 •
edited
Loading

Thanks for the feedback @bschaatsbergen

Good catch!

Fix is provided here: dimisjim@ea2a22a

@dimisjim dimisjim requested a review from bschaatsbergen April 21, 2024 10:42
@d-costa
Copy link
Collaborator

d-costa commented Apr 25, 2024

LGTM

@bschaatsbergen
Copy link
Member

Sorry for the delayed review on my side, I need to test whether this could be a breaking change or not.. have you already tested it on your side @dimisjim?

@dimisjim
Copy link
Author

@bschaatsbergen yes, works as expected

@dimisjim
Copy link
Author

Hello,

Is something blocking this?

@joe1981al
Copy link

Might I suggest also parsing cos_project from the input? It would be similar to what you've done but would be second item in the split, otherwise "cos-cloud" which is the default in the module being used. Not sure if necessary, but might be useful.

renovate bot and others added 10 commits November 13, 2024 14:06
@renovate @dimisjim
chore(deps): update planetscale/ghcommit-action action to v0.2.0
2f2525b 
Signed-off-by: dimisjim <[email protected]>
@renovate @dimisjim
chore(deps): update terraform-docs/gh-actions action to v1.3.0
bbf41a9 
Signed-off-by: dimisjim <[email protected]>
@github-actions @dimisjim
terraform-docs: automated action
d29525a 
Signed-off-by: dimisjim <[email protected]>
@cblkwell @dimisjim
Loosen module version requirements for container-vm
b8b0852 
Signed-off-by: cblkwell <[email protected]>
@cblkwell @dimisjim
Fixing README
4bd6019 
Signed-off-by: cblkwell <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@cblkwell @dimisjim
Adding enabled = true for iap backend
fcbf843 
Signed-off-by: cblkwell <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@cblkwell @dimisjim
Bumping version requirement due to iap enabled setting
877024b 
Signed-off-by: cblkwell <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@cblkwell @dimisjim
Fixing README
8eeb2eb 
Signed-off-by: cblkwell <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@cblkwell @dimisjim
Fixing README
0272b7f 
Signed-off-by: cblkwell <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@d-costa @dimisjim
chore: fix terraform docs behavior
2a869cf 
Signed-off-by: David Costa <[email protected]>
Signed-off-by: dimisjim <[email protected]>
@github-actions github-actions bot added the github-actions Pull requests that update Github Actions code label Nov 13, 2024
@dimisjim
Merge branch 'main' into patch-1
bc3f02c 
Signed-off-by: Dimitris Moraitidis <[email protected]>
@d-costa
Copy link
Collaborator

d-costa commented Nov 19, 2024

@bschaatsbergen can you take a look at this when possible? Would be great to merge this and release a new version

@d-costa
Copy link
Collaborator

d-costa commented Nov 27, 2024

Hey @dimisjim sorry for the delay. Could you please squash these commits? There are some that are not signed off as required by the DCO check.

@d-costa
Copy link
Collaborator

d-costa commented Dec 3, 2024

I'd also suggest making data "google_compute_image" "cos" conditional based on the presence of var.machine_image, to avoid reading it even when machine_type is specified

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bschaatsbergen bschaatsbergen bschaatsbergen approved these changes

Assignees

@d-costa d-costa

Labels
documentation Improvements or additions to documentation github-actions Pull requests that update Github Actions code
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Instance being replaced even when the machine_image is pinned
6 participants
@dimisjim @d-costa @bschaatsbergen @joe1981al @fossabot @cblkwell