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.

Sign up for GitHub

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

Jump to bottom

Trigger AWS runs via tower API #1160

Merged
merged 8 commits into from
Jul 9, 2021
Merged

Trigger AWS runs via tower API #1160

merged 8 commits into from
Jul 9, 2021

Conversation

ggabernet
Copy link
Member

@ggabernet ggabernet commented Jul 5, 2021 •
edited
Loading

  • Trigger AWS runs via Tower API

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

@ggabernet ggabernet changed the title Dev Trigger AWS runs via tower API Jul 5, 2021
KevinMenden
KevinMenden approved these changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@KevinMenden KevinMenden left a comment

Choose a reason for hiding this comment

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

Looks good to me, but that doesn't mean anything as I don't know much about AWS 😁 so maybe another review would be good

pontus
pontus approved these changes Jul 6, 2021
View reviewed changes
Copy link
Contributor

@pontus pontus left a comment

Choose a reason for hiding this comment

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

Looks sensible.

I don't seem to be able to properly understand the tower api docs, but possibly it might be worth making the effort to supply a proper dateCreated (alternatively if it's not mandatory, remove)?

@pontus
Copy link
Contributor

pontus commented Jul 6, 2021

Maybe should add that I don't really know tower either, but if it works, it works (for clarity; I haven't tested).

I'm also a bit curious about the style with supplying the workspaceId through the query string for a post, but guess that's a tower thing.

ewels
ewels approved these changes Jul 6, 2021
View reviewed changes
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Looks great! I feel like this might be a golden opportunity to create a dedicated GitHub Action for launching Tower though. I'm going to have a quick play to see if I can come up with something if that's ok..

nf_core/pipeline-template/.github/workflows/awsfulltest.yml Outdated Show resolved Hide resolved
@ggabernet
Copy link
Member Author

Maybe should add that I don't really know tower either, but if it works, it works (for clarity; I haven't tested).

I'm also a bit curious about the style with supplying the workspaceId through the query string for a post, but guess that's a tower thing.

Yes that has to do with the Tower API, nothing I can change here

ewels
ewels reviewed Jul 6, 2021
View reviewed changes
nf_core/pipeline-template/.github/workflows/awsfulltest.yml Outdated
Comment on lines 39 to 40
"paramsText" : "{\"outdir\":\"s3://'"${AWS_S3_BUCKET}"'/{{ short_name }}/results-'"${GITHUB_SHA}"'\"}",
"revision" : "'"${GITHUB_SHA}"'",
Copy link
Member

Choose a reason for hiding this comment

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

How come some of these are quoted with "'" and some are not?

Copy link
Member

Choose a reason for hiding this comment

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

Ah it's because env variables don't get into single-quoted strings, but you need that to avoid having to escape every ". Ok, lesser of evils maybe!

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah I played with different options on how to do proper escaping and variable setting, and that's the only option that was working for me

@ewels
Copy link
Member

ewels commented Jul 6, 2021

Ok so can hopefully now used this: https://github.com/marketplace/actions/tower-action

I took what you had here and wrapped it up into an action so that it's a little bit friendlier for pipeline devs to edit 😀

@ggabernet
Copy link
Member Author

I've updated the actions to use the tower-action that @ewels wrote now, tested it already with the Testpipeline and tests are passing there :)

@ggabernet ggabernet enabled auto-merge July 9, 2021 06:48
@ggabernet ggabernet merged commit 16b5b20 into nf-core:dev Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewels ewels ewels approved these changes

@pontus pontus pontus approved these changes

@KevinMenden KevinMenden KevinMenden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ggabernet @pontus @ewels @KevinMenden