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

hotfix: correct way of getting environment variables #5622

Merged
merged 3 commits into from May 20, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Apr 23, 2019

Ref: #5618
Related Comment: #5618 (comment)

Copy link
Member

@humitos humitos left a comment

Looks good.

Just a minor cleanup change to make and we can merge it.

"""Return env vars with their values of the project."""
env_vars = self.version.project.environmentvariable_set.values_list('name', 'value')
"""Return env vars with their values of the project as a dictionary."""
env_vars = self.version.project.environment_variables
Copy link
Member

@humitos humitos Apr 25, 2019

I'd say that you can remove this private method completely and access the variable directly where it's used since it's used just once.

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 25, 2019

Thank you.
I have pushed the changes.

@humitos
Copy link
Member

@humitos humitos commented Apr 25, 2019

Also, your new changes do not include the changes from your original PR, I think. Please, double check that.

@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Apr 25, 2019

@humitos

your new changes do not include the changes from your original PR, I think

I didn't understand. Original PR was #5540 and since #5618 is not merged yet, so the changes made in original PR are still there.

@humitos
Copy link
Member

@humitos humitos commented Apr 27, 2019

@dojutsu-user oh, you are right. It's should be merged in rel branch which is the one deployed to production.

I'm not sure what's the usual process here, @ericholscher

  1. merge this PR
  2. close #5618
  3. revert #5618 on rel

or...

  1. merge #5618
  2. revert #5618
  3. merge this PR

or something different?

@ericholscher
Copy link
Member

@ericholscher ericholscher commented Apr 28, 2019

Either one is fine.

Copy link
Member

@humitos humitos left a comment

Looks good!

@humitos humitos merged commit 1a686cd into readthedocs:master May 20, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the refix-env-var-json branch May 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants