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

don't uppercase envoverride #4458

Merged
merged 1 commit into from Aug 23, 2017

Conversation

Projects
None yet
4 participants
@xgaia
Copy link
Contributor

commented Aug 18, 2017

@erasche,

Sorry for the second PR,

I think it is better like this, Only default env var are uppercased, but user env (env_override) are not.

What do you think ?

@galaxybot galaxybot added the triage label Aug 18, 2017

@galaxybot galaxybot added this to the 17.09 milestone Aug 18, 2017

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

@galaxybot test this

@erasche

This comment has been minimized.

Copy link
Member

commented Aug 21, 2017

@xgaia yes, I like this better. Since the upper case env vars are effectively part of the GIE API contract, this at least gives authors more flexibility in passing vars to underlying software.

@erasche erasche merged commit a834381 into galaxyproject:dev Aug 23, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: JavaScript No alert changes
Details
@natefoo

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

Unfortunately, this change has broken automatic fetching of the dataset from which the GIE is launched since $additional_dataset_ids is now passed uncapitalized. However, I am not sure of the best solution here since it seems that both functionalities are needed (capitalize some env_override keys, don't capitalize others).

@natefoo natefoo referenced this pull request Oct 4, 2017

Open

GIE Hardening #4651

9 of 21 tasks complete
@xgaia

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2017

Hello @natefoo ,

maybe we can replace dataset_hid and additional_dataset_ids by DATASET_HID and ADDITIONAL_DATASET_IDS in all the mako files.

here

I can make the PR if it's OK

@natefoo

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

I thought about this, but some of these are used lowercase elsewhere so I am not sure if it's entirely safe.

@natefoo

This comment has been minimized.

Copy link
Member

commented Oct 5, 2017

@xgaia see #4760 for a proposed solution.

@xgaia

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

sounds good to me 👍

@xgaia xgaia deleted the xgaia:interactive_environment branch Oct 6, 2017

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.