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

[17.05] Validate workflow step after step argument injection #4483

Merged
merged 1 commit into from Aug 25, 2017

Conversation

Projects
None yet
4 participants
@guerler
Copy link
Contributor

commented Aug 23, 2017

Fixes #4481. This PR makes sure that the check and update helper for workflow steps is called after all step arguments have been injected into the step state. Previously the first check was performed before injecting optional user inputs, solely relying on the inputs provided in the database/configuration state. In some cases this could led to a premature upgrade message response before considering the inputs parsed through the step arguments dictionary. There might be still some issues with this PR, particularly with regard to conflicting messages between the validations. I am looking into those now.

@guerler guerler added this to the 17.05 milestone Aug 23, 2017

@guerler guerler changed the title [17.05] Validate workflow step after step argument injection [WIP] Validate workflow step after step argument injection Aug 23, 2017

@guerler guerler changed the base branch from dev to release_17.05 Aug 23, 2017

@nsoranzo nsoranzo requested a review from jmchilton Aug 23, 2017

@nsoranzo nsoranzo changed the title [WIP] Validate workflow step after step argument injection [WIP][17.05] Validate workflow step after step argument injection Aug 23, 2017

@nsoranzo nsoranzo modified the milestones: 17.09, 17.05 Aug 23, 2017

@guerler guerler modified the milestones: 17.05, 17.09 Aug 23, 2017

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

@nsoranzo Should we target 17.05 or 17.09? I think either would be fine, so its up to you guys.

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

We always use the next-relase milestone (unless it's a backport), so that all PRs are found by the script that generate the release notes, as @martenson explained to me.

@guerler guerler changed the title [WIP][17.05] Validate workflow step after step argument injection Validate workflow step after step argument injection Aug 23, 2017

@guerler guerler changed the base branch from release_17.05 to dev Aug 23, 2017

@guerler guerler modified the milestones: 17.09, 17.05 Aug 23, 2017

@guerler

This comment has been minimized.

Copy link
Contributor Author

commented Aug 23, 2017

I see. Cool. Thanks.

@guerler guerler added status/review and removed status/WIP labels Aug 23, 2017

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

I may have used the wrong terms above, I've edited to clarify. It is correct to target a release_XX.XX Git branch for bugfixes, but use the next-release milestone on the right (unless it's a backport).

In practice, I think this PR should target the release_17.05 branch and have [17.05] in the title, but 17.09 as milestone. I know it's confusing, but I suppose that fixing the script would require some work.

@guerler guerler changed the base branch from dev to release_17.05 Aug 23, 2017

@guerler guerler changed the title Validate workflow step after step argument injection [17.05] Validate workflow step after step argument injection Aug 23, 2017

@nsoranzo

This comment has been minimized.

Copy link
Member

commented Aug 23, 2017

@tshtatland

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2017

Tested this change on our local installation. Works fine, issue fixed:
Workflow fails to run after optional input is added to tool · Issue #4481 · galaxyproject/galaxy
Many thanks for the quick response, this fix helps us and our Galaxy users a lot.

@dannon dannon merged commit c709264 into galaxyproject:release_17.05 Aug 25, 2017

6 checks passed

api test Build finished. 275 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 148 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 34 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.