-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Adjust the servers to be always linux instances #25172
base: main
Are you sure you want to change the base?
Conversation
e7fdb38
to
3539e4e
Compare
07fa5d7
to
be7e7e7
Compare
enos/enos-scenario-upgrade.hcl
Outdated
@@ -142,16 +145,16 @@ scenario "upgrade" { | |||
Bring the new upgraded binary from the artifactory to the instance running enos. | |||
EOF |
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.
This description is no longer true as far as I can tell. It doesn't download the binary.
module fetch_artifact { | ||
for_each = toset(distinct(var.oss)) | ||
source = "../fetch_artifactory" | ||
|
||
artifactory_username = var.artifactory_username | ||
artifactory_token = var.artifactory_token | ||
artifactory_host = var.artifactory_host | ||
artifactory_repo = var.artifactory_repo | ||
edition = var.edition | ||
os = each.value | ||
product_version = var.product_version | ||
arch = var.arch | ||
download_binary = var.download_binaries | ||
download_binary_path = "${var.download_binaries_path}/${each.value}" | ||
} |
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.
Calling this module install_binaries
is really confusing me. If download_binary = true
, it only downloads the binaries locally and doesn't install them anywhere. If download_binaries = false
, then as far as I can tell the only thing it does is expose the outputs from data.eno_artifactory_item.nomad
.
It seems like the only reason this module exists is to handle the for_each
on var.oss
. Maybe we can push that down into fetch_artifactory
instead and avoid having to come up with a name 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.
the logic to get the correct url for each binary is not trivial, and it will probably grow, I think we can benefit from having it in its own module
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.
Ok, no problem. let's merge this and we can iterate as we extend this. Can we at least change the name though?
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.
LGTM! 👍
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.
LGTM!
While testing #25172 I found a few spots where #25152 wasn't capturing the errors from transient failures correctly or exiting early instead of retrying. Ref: https://hashicorp.atlassian.net/browse/NET-11546
Description
Becase there is not many OS specific behaviour on the servers code, they can always be run on linux instances, this PR introduces that change and fixes some errors in the update of the last server.
Testing & Reproduction steps
Links
Contributor Checklist
changelog entry using the
make cl
command.ensure regressions will be caught.
and job configuration, please update the Nomad website documentation to reflect this. Refer to
the website README for docs guidelines. Please also consider whether the
change requires notes within the upgrade guide.
Reviewer Checklist
backporting document.
in the majority of situations. The main exceptions are long-lived feature branches or merges where
history should be preserved.
within the public repository.