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
Merged

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

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

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

Changes are good to me!

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

Choose a reason for hiding this comment

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

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

@dojutsu-user
Copy link
Member Author

@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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Defaulting to None is fine.

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

Copy link
Member

Choose a reason for hiding this comment

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

The internal method returns the hash of the empty string

Copy link
Member Author

@dojutsu-user dojutsu-user Apr 10, 2019

Choose a reason for hiding this comment

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

@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 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
@dojutsu-user dojutsu-user deleted the env-var-json branch April 22, 2019 17:16
ericholscher added a commit that referenced this pull request Apr 22, 2019
This reverts commit c5e705e, reversing
changes made to a992ad1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-wipe when env var is added/deleted
3 participants