-
Notifications
You must be signed in to change notification settings - Fork 57
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
(BKR-831) added dynamic puppet-agent version read from master #19
Conversation
Refer to this link for build results (access rights to CI server needed): |
@puppetlabs/beaker ready for review! |
# @param [Hash{Symbol=>String}] local_options local method options hash | ||
# | ||
# @return [String] puppet-agent version to install | ||
def get_puppet_agent_version(host, local_options={}) |
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.
A few comments here, not all that need to be addressed in this PR:
-
We continue to call things
puppet_*
when we really mean AIO. We should really figure out a way to clean that so that we can say what we mean. </end rant> -
This method uses the
aio_agent_build
key that puppet facts returns; from what I can tell, we actually want to useaio_agent_version
that is identical toaio_agent_build
except for it doesn't contain the git sha for the version number.
Example build: "aio_agent_build": "1.5.3.245.g9bd94ac"
Example version: "aio_agent_version": "1.5.3.245"
My understanding is that we could use the AIO version to build a path to the tarballs to download; I'm unclear on how we would use the build number in this way.
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.
@tvpartytonight for your first point, I think we're somewhat inconsistent for sure about the terms. I think it's fair to say that puppet_agent
is a container term that would hold all instances of aio
as well as agents outside of aio
, so it's used when we want to be a little more general than that. But we haven't been 100% about that, because the installers that were added for aio
when that was going down were all called puppet_agent
as well. Since we're coming closer and closer to EOL'ing the 3.8 series from before aio
came out that this distinction is becoming less and less meaningful, however, so I'm fine just letting it fall away naturally. But it would be good to have a more explicit label on these things so I'm open to moving. Unfortunately the challenge to that has always been changing signatures and hence the API, but perhaps we can get around that with other methods.
For your second comment, I've done a little looking, and commented on this asking @jpartlow for more info in the ticket. Hopefully we can resolve that there, and figure out what the right move is for this code.
do not merge, as there's conversation in the ticket that needs to be resolved before this happens |
6861269
to
7a0884f
Compare
@tvpartytonight, the comments in the ticket make it seem that |
Refer to this link for build results (access rights to CI server needed): |
um, couldn't find beaker on our mirror? @puppetlabs-jenkins retest this please |
Refer to this link for build results (access rights to CI server needed): |
@puppetlabs-jenkins retest this please |
Refer to this link for build results (access rights to CI server needed): |
@puppetlabs-jenkins retest this please |
Refer to this link for build results (access rights to CI server needed): |
7a0884f
to
0aee1cb
Compare
Refer to this link for build results (access rights to CI server needed): |
Refer to this link for build results (access rights to CI server needed): |
@puppetlabs-jenkins retest this please |
Refer to this link for build results (access rights to CI server needed): |
0aee1cb
to
2e6d782
Compare
Got weird results w/really old versions of beaker-pe, etc. suspect that it has something to do with the PR being old and targeted to pre-1.0 beaker-pe. Rebased & re-pushed. We'll see what that does to the test results when they're auto-kicked. |
Refer to this link for build results (access rights to CI server needed): |
@puppetlabs/beaker @mwbutcher review? |
# before the agents | ||
facts_result = on(master, 'puppet facts') | ||
facts_hash = JSON.parse(facts_result.stdout.chomp) | ||
puppet_agent_version = facts_hash['values']['aio_agent_build'] |
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.
For released versions of Puppetserver, it looks like there isn't an aio_agent_build
value, just an aio_agent_version
. It seems like this will be a problem when trying to install released versions? I'm unclear though, as there might be logic I am missing that only allows this code path to execute if we know we have a development build...
Gist with relevant info.
2e6d782
to
6727879
Compare
Refer to this link for build results (access rights to CI server needed): |
spec tests are failing due to unrelated work. The fix has been submitted in #42. This work can wait for that change and rebase to get green for merging. |
6727879
to
d42ef8e
Compare
Refer to this link for build results (access rights to CI server needed): |
👍 LGTM |
No description provided.