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

[WIP] Remote triggering a pipeline #158

Merged

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Jan 27, 2019

So for now a couple of things:

  • The auto user needs to be undeletable like the admin user. Still needs to be modifiable because the token might need to be updated
  • View/Edit the token for a pipeline
  • Write Tests!!
  • Make the end-point more secure. Right now I'm not checking if the trigger was by the auto user with the correct auto token
  • Deal with Pipeline parameters

Manual:

~/goprojects/gaia
❯ curl -X POST http://127.0.0.1:8080/api/v1/pipeline/1/4565632f-a77b-551b-b0cb-67e74dc9b15a/trigger
Trigger successful for pipeline: 1

@Skarlso Skarlso added the Needs Work The PR still requires some work from the submitter. label Jan 27, 2019
@codecov-io
Copy link

codecov-io commented Jan 27, 2019

Codecov Report

Merging #158 into master will increase coverage by 0.13%.
The diff coverage is 67.3%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #158      +/-   ##
==========================================
+ Coverage   66.38%   66.52%   +0.13%     
==========================================
  Files          32       33       +1     
  Lines        2490     2593     +103     
==========================================
+ Hits         1653     1725      +72     
- Misses        639      654      +15     
- Partials      198      214      +16
Impacted Files Coverage Δ
workers/pipeline/create_pipeline.go 44.08% <100%> (+0.6%) ⬆️
handlers/handler.go 85.18% <100%> (+0.87%) ⬆️
security/secret_generator.go 100% <100%> (ø)
handlers/auth.go 94.91% <100%> (+0.08%) ⬆️
handlers/user.go 39.68% <47.36%> (+5.1%) ⬆️
handlers/pipeline.go 53.94% <62.06%> (+2.77%) ⬆️
store/store.go 68.57% <85.71%> (+4.28%) ⬆️
workers/scheduler/scheduler.go 68.54% <0%> (-0.34%) ⬇️

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 ddf8c49...cc38b64. Read the comment docs.

@Skarlso Skarlso force-pushed the issues_151_remote_pipeline_trigger branch from 641fdcf to fcd7fc3 Compare January 29, 2019 05:49
@Skarlso
Copy link
Member Author

Skarlso commented Feb 1, 2019

Alright, triggering works:

❯ curl -X POST -H "Authorization: Basic YXV0bzo0YzE4YTkyNi1kNWE2LTVlYTMtODY5OC1jNmQ4NDllZGFiMDE=" http://127.0.0.1:8080/api/v1/pipeline/3/9e77e732-b0ce-54d8-a2c7-c45b9c99f9c0/trigger
Trigger successful for pipeline: 3

This made the above pipeline execute:

screenshot 2019-02-01 at 15 54 11

Alright, now for arguments:

❯ curl -X POST -H "Authorization: Basic YXV0bzo0YzE4YTkyNi1kNWE2LTVlYTMtODY5OC1jNmQ4NDllZGFiMDE=" -H "content-type:application/json" -d '{"args": [{"type": "String","key": "username","value": "ASDFUSER"}]}' http://127.0.0.1:8080/api/v1/pipeline/5/c1741ea3-2d28-53e1-9b17-a002b8673206/trigger
Trigger successful for pipeline: 5

Came over:

2019-02-01T16:50:40.540+0100 [DEBUG] plugin.arg1test_golang: 2019/02/01 16:50:40 Key: username; Value: ASDFUSER;

@Skarlso
Copy link
Member Author

Skarlso commented Feb 1, 2019

Alright... was sending wrong json data. :/ To my defense the error from echo wasn't really helpful:

 curl -X POST -H "Authorization: Basic YXV0bzo0YzE4YTkyNi1kNWE2LTVlYTMtODY5OC1jNmQ4NDllZGFiMDE=" -H "content-type:application/json" -d '[{"type": "String","key": "username","value": "ASDFUSER"}]' http://127.0.0.1:8080/api/v1/pipeline/5/c1741ea3-2d28-53e1-9b17-a002b8673206/trigger

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

@michelvocks How do you like the display of the tokens?
screenshot 2019-02-02 at 11 48 26
screenshot 2019-02-02 at 11 40 43

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

WTH. Somehow I messed up the handlers? :O

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

@michelvocks Rudimentary user token generation:
screenshot 2019-02-02 at 12 20 02
screenshot 2019-02-02 at 12 20 06
screenshot 2019-02-02 at 12 21 13

The slider is the token re-generation part.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

Oh, I just realised I don't need a new window... I can slide the user password thingy. I don't need a new icon.

Oh but I do, since it's a separate endpoint. And I need different actions. Not just a put which would save the pipeline. Unless I redesign the whole reset process 🤔

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

Ah, I'll only allow token reset for auto user. Since the reset saves the user with the encrypted password. Thus it will no longer match what was there previously.

@michelvocks
Copy link
Member

@Skarlso I'm not sure if the overview page is the right place for the token display. This information is usually needed once (or twice) but it's not a piece of information which you need regularly.

In my opinion, we need an information view in the pipeline details page. Currently, when you open the details page for pipeline you basically see no information about the pipeline (e.g. Name of the pipeline, last successful run, last failed run etc.).

Regarding the Auto User: Not quite sure if we should store for every user a UUID when you can only use it for one specific user. And the Auto User needs to be created by the user? Why we are not creating it automatically?

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

@michelvocks I agree on the pipeline view. It's not the best place.

I am generating the auto user. 🙂 Also agreed that that should be the only user with a token. Hence I'm restricting resetting tokens for any other user. Also I'm disallowing deleting this user. I haven't implemented that yet.

@Skarlso
Copy link
Member Author

Skarlso commented Feb 2, 2019

So, regarding the details view, I kind of want to do that in a separate issue.

I think it's a good idea to create a nice details view for pipelines... but I'm unsure I would manage to make that pretty. :D

@Skarlso Skarlso added needs review and removed Needs Work The PR still requires some work from the submitter. labels Feb 3, 2019
@Skarlso Skarlso requested a review from michelvocks February 3, 2019 21:26
@Skarlso
Copy link
Member Author

Skarlso commented Feb 3, 2019

@michelvocks alright. Added the last of the coverage. I think this is ready for a review. 👍

frontend/client/views/settings/index.vue Outdated Show resolved Hide resolved
frontend/client/views/settings/index.vue Outdated Show resolved Hide resolved
frontend/client/views/settings/index.vue Outdated Show resolved Hide resolved
workers/pipeline/create_pipeline.go Outdated Show resolved Hide resolved
Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

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

LGTM ❤️ Thanks a lot @Skarlso 🤗

@Skarlso
Copy link
Member Author

Skarlso commented Feb 4, 2019

@michelvocks Np! Thanks! :)

@Skarlso
Copy link
Member Author

Skarlso commented Feb 5, 2019

@michelvocks Done. What do you think?
security/secret_generator.go -- this is the one.

@michelvocks
Copy link
Member

Perfect, thanks @Skarlso
Feel free to merge it.

@Skarlso Skarlso merged commit 6987967 into gaia-pipeline:master Feb 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants