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 8 commits into
base: main
Choose a base branch
from
Open

Fix pinning of machine_type #149

wants to merge 8 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

@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.

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

@bschaatsbergen bschaatsbergen Awaiting requested review from bschaatsbergen

Requested changes must be addressed to merge this pull request.

Assignees

@d-costa d-costa

Labels
documentation Improvements or additions to documentation
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
4 participants
@dimisjim @d-costa @bschaatsbergen @joe1981al