-
Notifications
You must be signed in to change notification settings - Fork 9
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
Brew CI: try checking out matching branches #575
Conversation
Part of #564. Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
@@ -61,9 +61,14 @@ echo '# BEGIN SECTION: setup the osrf/simulation tap' | |||
brew tap osrf/simulation | |||
echo '# END SECTION' | |||
|
|||
if [[ -n "${PULL_REQUEST_URL}" ]]; then | |||
echo "# BEGIN SECTION: pulling ${PULL_REQUEST_URL}" | |||
brew pull ${PULL_REQUEST_URL} |
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 brew pull command has been deprecated
So there's no use for PULL_REQUEST_URL
anymore? Should it be removed?
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.
it's not used in this job, but I think it is used in a downstream job that updates the bottle hash sha256
values
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.
confirmed that it is still in use in the downstream job
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.
confirmed that it is still in use in the downstream job
If I read correctly the DSL code:
parameters {
currentBuild()
predefinedProp("PULL_REQUEST_URL", "https://github.com/osrf/homebrew-simulation/pull/\${ghprbPullId}")
}
Looks to me like we are overriding PULL_REQUEST_URL
value to the base URL + ghprbPullId
instead of using the current one in this script (injected by currentBuild()
.
If we remove PULL_REQUEST_URL
from here, is there anyway of launching the job manually? Can that be done without these changes? Maybe is not a supported use case.
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, maybe some of this logic is left over from when release.py
could trigger a bottle build and would pass PULL_REQUEST_URL
. Now we only start bottle builds from GitHub comments. I'll see if there's something that should be removed
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.
If we remove PULL_REQUEST_URL from here, is there anyway of launching the job manually? Can that be done without these changes? Maybe is not a supported use case.
it's not easy to start the "bottle hash updater job" manually since it needs artifacts from a bottle build
I looked and I don't think there's anything more to be changed
echo "# BEGIN SECTION: pulling ${PULL_REQUEST_URL}" | ||
brew pull ${PULL_REQUEST_URL} | ||
if [[ -n "${ghprbSourceBranch}" ]] && \ | ||
[[ "${ghprbSourceBranch}" =~ matching_branch\/ ]] |
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.
we need to update the regexp for matching branches accordingly to #572. It would be great if we can avoid the pattern duplication.
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.
updated in 8a9b0c9
I suppose we could set a variable in dependencies_archive.sh
, which would work for homebrew and Ubuntu jobs on build.osrfoundation.org, but not on Windows. We could encode the pattern in a file? It seems simpler to just duplicate it for now
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 multi-platform support that works on Win and UNIX I can not think in a different option than using a file, yes. Would be great to have this kind of support but by now I'm more worried about having the same regexp in two different files which is a call for problems in the future.
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.
An alternative to storing the regex in a file is a python script that takes a string as an input and compares it to the hard-coded regex. this would be similar to our detect_cmake_major_version.py script that we use on multiple platforms.
So please let me know if this is a prerequisite for merging or if I should open an issue to track the desire for a cross-platform script for identifying ci_matching_branch
es.
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'll open an issue for this in order to move forward
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.
Signed-off-by: Steve Peters <[email protected]>
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
Part of #564, similar to #572.
Tested with
The
brew pull
command has been deprecated, so that code was replaced with checking for matching branches.