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

Update readthedocs-environment.json file when env vars are added/deleted #5540

Merged
merged 14 commits into from Apr 22, 2019

Conversation

@dojutsu-user
Copy link
Member

@dojutsu-user dojutsu-user commented Mar 26, 2019

Closes #5249

Copy link
Member

@humitos humitos left a comment

Thanks for your PR.

I think we should use what we already have at PythonEnvironment class that handle most of these there and just add a new check instead of duplicating the efforts in EnvironmentVariable model.

readthedocs/projects/models.py Outdated Show resolved Hide resolved
Copy link
Member

@humitos humitos left a comment

Hey! Changes look way better now!

I left some comments to improve the logic, but I think this is really close.

readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
readthedocs/rtd_tests/tests/test_doc_building.py Outdated Show resolved Hide resolved
readthedocs/rtd_tests/tests/test_doc_building.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved
@dojutsu-user
Copy link
Member Author

@dojutsu-user dojutsu-user commented Mar 29, 2019

@stsewd
Thank you.
I have added the improvements suggested by you.

humitos
humitos approved these changes Apr 1, 2019
Copy link
Member

@humitos humitos left a comment

Thanks for working on this!

Changes are good to me!

stsewd
stsewd approved these changes Apr 1, 2019
def _get_env_vars(self):
"""Return tuple of tuples of env vars with their values of the project."""
env_vars = self.version.project.environmentvariable_set.values_list('name', 'value')
return tuple(env_vars)
Copy link
Member

@stsewd stsewd Apr 1, 2019

I don't think this is needed (cast explicitly to a tuple)

@dojutsu-user
Copy link
Member Author

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

@stsewd @humitos
Thank you for guiding me on this.
I have been learning constantly from you two.

@@ -174,6 +176,7 @@ def is_obsolete(self):

env_python = environment_conf.get('python', {})
env_build = environment_conf.get('build', {})
env_vars_hash = environment_conf.get('env_vars_hash', None)
Copy link
Member

@stsewd stsewd Apr 2, 2019

Defaulting to None will cause to every build before this change to wipe. Not really sure if that will be a problem, but we could default to the hash of an empty string here to prevent that.

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 2, 2019

Yeahhh.. that is better.
Should I generate it every time or should I copy the value from the web for the sha256 hash of empty string?

Copy link
Member

@humitos humitos Apr 2, 2019

Defaulting to None is fine.

The internal method should return None as well if there are no Environment Variables.

Copy link
Member

@stsewd stsewd Apr 2, 2019

The internal method returns the hash of the empty string

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 10, 2019

@humitos @stsewd
What should we do here?
I am agreeing with @stsewd idea here. Returning the hash of empty string saves us few lines of code.

@humitos
Copy link
Member

@humitos humitos commented Apr 22, 2019

I understand that this PR is ready to merge.

I think the last thing we talked here (#5540 (comment)) can be left as is it right not in the PR.

@humitos humitos merged commit c5e705e into readthedocs:master Apr 22, 2019
1 check passed
@dojutsu-user dojutsu-user deleted the env-var-json branch Apr 22, 2019
ericholscher added a commit that referenced this issue Apr 22, 2019
This reverts commit c5e705e, reversing
changes made to a992ad1.
ericholscher added a commit that referenced this issue May 30, 2019
agjohnson added a commit that referenced this issue May 30, 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.

3 participants