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.

Sign up for GitHub

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

Jump to bottom

Unify build_env and setup_env #5672

Closed
stsewd opened this issue May 7, 2019 · 7 comments · Fixed by #8815
Closed

Unify build_env and setup_env #5672

stsewd opened this issue May 7, 2019 · 7 comments · Fixed by #8815
Assignees
@stsewd
Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Milestone
Refactoring

Comments

@stsewd
Copy link
Member

stsewd commented May 7, 2019

Currently we are using two different build instances

https://github.com/rtfd/readthedocs.org/blob/2e797fa1b45fe9772c6cf77c4fd55731d515585d/readthedocs/projects/tasks.py#L454-L454

https://github.com/rtfd/readthedocs.org/blob/2e797fa1b45fe9772c6cf77c4fd55731d515585d/readthedocs/projects/tasks.py#L536

We have some places where we are expecting for the setup_env attr.

This should be only one instance.
@stsewd stsewd added the Improvement Minor improvement to code label May 7, 2019
@stsewd stsewd added this to the Refactoring milestone May 7, 2019
@stsewd stsewd self-assigned this May 7, 2019
@stsewd stsewd mentioned this issue May 7, 2019
Closed
@humitos
Copy link
Member

humitos commented May 8, 2019

I'm not sure to follow this issue.

  • What are the current problems?
  • What are the benefits of using the same build environment?

@stsewd
Copy link
Member Author

stsewd commented May 8, 2019

Code is hard to read and follow, the setup_env is expected in a parent class. Also, sometimes we are setting the environment to None, but we always need an environment. Also that we sometimes expect a dictionary and others an object.

We could still use separated environments, because we don't know the docker image to use at the clone step (this is known after parsing the config file). But the rest of things need to be refactored.

stsewd added a commit to stsewd/readthedocs.org that referenced this issue May 8, 2019
@stsewd
Fix bug in notifications
ace8325 
If an exception is raised before setting the self.build and self.version
objects, we ended up with those having the value of `{}` (empty dict).

In send_notification we expect to have these objects initialized.
We should refactor this to only have one point where to catch exceptions
and send the notification from there. Related to readthedocs#5672
@stsewd stsewd mentioned this issue May 8, 2019
Merged
stsewd added a commit to stsewd/readthedocs.org that referenced this issue May 14, 2019
@stsewd
Simplify lock acquire
2f5f4da 
Currently we are acquiring 3 locks.
We should only have 2 (we can't have only one because of readthedocs#5672)
This was referenced May 14, 2019
Merged
Merged
@humitos
Copy link
Member

humitos commented May 16, 2019

We have to be careful with this because on setup_env we are passing update_on_success=False to avoid changing the state of the build and we are only passing environment= to the build_env (although, I'm not sure if it could be a problem)

@stale
Copy link

stale bot commented Jun 30, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Jun 30, 2019
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Jun 30, 2019
@stale
Copy link

stale bot commented Aug 15, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Aug 15, 2019
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Aug 15, 2019
@stale
Copy link

stale bot commented Sep 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Sep 29, 2019
@stsewd stsewd removed the Status: stale Issue will be considered inactive soon label Sep 30, 2019
@stale
Copy link

stale bot commented Nov 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Nov 14, 2019
@stsewd stsewd added Accepted Accepted issue on our roadmap and removed Status: stale Issue will be considered inactive soon labels Nov 14, 2019
@humitos humitos mentioned this issue Jan 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stsewd stsewd

Labels
Accepted Accepted issue on our roadmap Improvement Minor improvement to code
Projects
None yet
Milestone
Refactoring
Development

Successfully merging a pull request may close this issue.

Build process: use Celery handlers readthedocs/readthedocs.org
2 participants
@humitos @stsewd