-
Notifications
You must be signed in to change notification settings - Fork 659
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(config/travis): configurable regexes to parse artifacts #142
Conversation
0a00091
to
2ed0d79
Compare
Sorry for the multiple pushes, I am unable to build locally due to:
|
2ed0d79
to
c4cef9d
Compare
@tomaslin @gardleopard let me know if this needs any improvements, thanks! |
I would like it to keep the one there as the default and have a configuration override for adding custom parsers. ie some configurable parser setting for each travis server in https://github.com/spinnaker/igor/blob/master/igor-web/config/igor.yml |
Thanks @gardleopard, that makes sense. I have this in my queue and will refactor as you suggest as soon as possible. |
c4cef9d
to
e1e5b90
Compare
@gardleopard @tomaslin let me know how this looks 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.
This looks like a good solution to me, nice work @srvaroa !
I would like you to add an example of it into this file as well: https://github.com/spinnaker/igor/blob/master/igor-web/config/igor.yml#L17
static List<GenericArtifact> getArtifactsFromLog(String buildLog, Iterable<String> additionalRegexes) { | ||
final List<GenericArtifact> artifacts = new ArrayList<GenericArtifact>() | ||
final Set<String> regexes = new HashSet<>() | ||
regexes.addAll(DEFAULT_REGEXES) |
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 would prefer if injected regexes replaces the default ones. In some cases you would want to not use the standard regexes.
README.md
Outdated
Igor requires redis server to be up and running. | ||
|
||
Start Igor via `./gradlew bootRun`. Or by following the instructions using the [Spinnaker installation scripts](https://www.github.com/spinnaker/spinnaker). | ||
# Igor |
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.
Why is all this text replaced? You have not changed it all.
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 didn't realize the file has DOS line endings and I have ff=UNIX in my vimrc. I'll respect the current settings.
README.md
Outdated
by using a `regexes` list: | ||
|
||
``` | ||
# regexes: |
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 would move this up to travis so the placement for this is clear.
e1e5b90
to
8185869
Compare
@gardleopard I had the example already at igor-web/config/igor.yml but let me know if I'm missing the right place. |
is there any updates on this issue @gardleopard ? |
8185869
to
1842e5e
Compare
Needs tests for parsing the yaml configuration, could not get it to work in our environment. I tried with:
|
Thanks @gardleopard, I will add some extra checks. |
1842e5e
to
f8abc51
Compare
@gardleopard I have been starting the service with gradle run and different values in igor.yml. I think the mistake was just in the sample values provided, Groovy accepts / when declaring string literals so they are fine in the test. But in the yaml they are taken as part of the string. Just removing the surrounding / gets the regexes correctly loaded into the TravisService. I updated this in igor.yml and made minor changes to the PR, but it's mostly as-is. Could you try with any of these config values to verify?
(Unrelated to this PR, but I noticed that the sample values for |
This is good to go for me.
The @tomaslin This is PR is ready to be merged in my opinion. |
LGTM |
Can you change the commit message so it fits within the spinnaker commit format? http://www.spinnaker.io/docs/how-to-submit-a-patch#section-commit-message-conventions |
Offer configurable regexes to find RPM/DEB artifacts pushed to artifactory as the default ones only match on the output of the "art" CLI, but wouldn't on that of any other tools. The result is a failed Spinnaker pipeline when no artifact is found. With this change users may add a list of arbitrary regexes that will be used to parse build logs. An example is provided that corresponds to Gradle maven-publish, which would match on the following line (taken from a real build, with project names changed): Upload https://artifacts.my.org/artifactory/yum-local/some/project/package-0.1.0.20161216.g1b1c767.rpm
f8abc51
to
b025018
Compare
Thanks @tomaslin , @gardleopard - I've updated the PR & commit, let me know if I missed any detail. |
travis: configurable regexes to parse artifacts
Offer configurable regexes to find RPM/DEB artifacts pushed to
artifactory as the default ones only match on the output of the "art"
CLI, but wouldn't on that of any other tools. The result is a failed
Spinnaker pipeline when no artifact is found.
With this change users may add a list of arbitrary regexes that will be
used to parse build logs. An example is provided that corresponds to
Gradle maven-publish, which would match on the following line (taken
from a real build, with project names changed):