-
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: Add flag to disable downloading tf for airgapped environments #2843
feat: Add flag to disable downloading tf for airgapped environments #2843
Conversation
Ah, missed that |
Hi @adam-verigin thank you for the change. Yes, please either rebase or merge the default branch and resolve conflicts. |
8417b8d
to
e10ed6d
Compare
resp, err := http.Get(url) | ||
if err != nil || resp.StatusCode != 200 { | ||
return nil, fmt.Errorf("Unable to list Terraform versions: %s", err) | ||
} |
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.
gosec alerts
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've added a #nosec
comment here for this particular rule (G107
) because terraform-switcher makes (AFAICT) the exact same call a few lines later. I guess that since their call is in a library and this one is not, it makes gosec unhappy.
I'm open to alternative suggestions if you have any.
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.
unsure what the best strategy is here.
Is there an alternative beside setting the nosec comment?
cc @runatlantis/maintainers
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.
G107 handle untrusted url, but this url is provided by server configuration. so it's ok to ignore it.
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.
otherwise lgtm
resp, err := http.Get(url) | ||
if err != nil || resp.StatusCode != 200 { | ||
return nil, fmt.Errorf("Unable to list Terraform versions: %s", err) | ||
} |
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.
G107 handle untrusted url, but this url is provided by server configuration. so it's ok to ignore it.
If you're both content with the gosec work-around (discussed here), I think I've addressed all open feedback on this 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.
Thank you! Your refactoring code is also helpful.
Thank you @adam-verigin ! Please feel free to propose more changes! |
@nitrocode @krrrr38 Thank you both for the thorough review! This PR ended up in a better place because of it 😄 |
what
terraform_client.go
why
--tf-download-url
configuration optionFrom #2701, this does not address the
os.Exit()
calls in https://github.com/warrensbox/terraform-switcher/blob/107631ffa4108d172909149d295c64c08cbedf00/lib/list_versions.go#L109-L139, though it does equip the code to handle it if an upstream change is made to return errors rather than exiting.references