Skip to content
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

Pipeline parameters #71

Merged
merged 2 commits into from
Aug 15, 2018

Conversation

michelvocks
Copy link
Member

Fixes #24

@michelvocks michelvocks added the Needs Work The PR still requires some work from the submitter. label Aug 9, 2018
@michelvocks michelvocks self-assigned this Aug 9, 2018
@codecov-io
Copy link

codecov-io commented Aug 9, 2018

Codecov Report

Merging #71 into master will increase coverage by 0.2%.
The diff coverage is 62.5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master      #71     +/-   ##
=========================================
+ Coverage   63.52%   63.72%   +0.2%     
=========================================
  Files          19       19             
  Lines        1653     1668     +15     
=========================================
+ Hits         1050     1063     +13     
- Misses        492      495      +3     
+ Partials      111      110      -1
Impacted Files Coverage Δ
handlers/pipeline.go 39.25% <100%> (+0.91%) ⬆️
services/service_provider.go 77.77% <100%> (ø) ⬆️
scheduler/scheduler.go 73.3% <55%> (-1.01%) ⬇️
security/vault.go 76.69% <0%> (+3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22073ca...531a815. Read the comment docs.

@michelvocks michelvocks changed the title [WIP] Pipeline parameters Pipeline parameters Aug 10, 2018
@michelvocks michelvocks removed the Needs Work The PR still requires some work from the submitter. label Aug 10, 2018
@michelvocks
Copy link
Member Author

ping @Skarlso
It's ready for review. We should merge #67 before so this PR doesn't include it anymore.
For testing you can use following pipeline: https://github.com/michelvocks/gaia-test-pipeline/tree/dependson

@michelvocks michelvocks requested a review from Skarlso August 10, 2018 22:50
@Skarlso
Copy link
Member

Skarlso commented Aug 11, 2018

Excellent! Looks good on a first run, I'll give it a manual. :)

@Skarlso
Copy link
Member

Skarlso commented Aug 11, 2018

So, unfortunately I ran into a couple of problems @michelvocks. :/

I created two pipelines with the same repository. Once I set values to one, the other just flashed in the argument window and then puff, made it disappear immediately and went on running the pipeline. It came out as empty value in the logs.

Second, the variables aren't cleared after once specifying them. Meaning, once you defined them, you can't every define them again, because they are already saved in cache I think, and so the asking module just flashes in and then immediately disappears.

The Vault stuff is working nicely however. :)

@michelvocks
Copy link
Member Author

Oh thanks for the report. Let me try to reproduce and fix it 🤗

@michelvocks
Copy link
Member Author

 @Skarlso I rebased and fixed your reported bugs. Try it again and let me know 🤗
I also updated my test pipeline to point back to gaia-pipeline repositories: https://github.com/michelvocks/gaia-test-pipeline/tree/master

Copy link
Member

@Skarlso Skarlso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work buddy! :)

@Skarlso Skarlso added the Ready To Merge PR is ready to be merged into master label Aug 15, 2018
@michelvocks michelvocks merged commit 2c57eea into gaia-pipeline:master Aug 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready To Merge PR is ready to be merged into master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants