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

Expose environment variables from database into build commands #4894

Merged
merged 6 commits into from
Nov 13, 2018

Conversation

humitos
Copy link
Member

@humitos humitos commented Nov 12, 2018

Environment variables can be added from the Admin and they will be used when running build commands. All the variables for that particular project will be expose to all the commands.

This is the first approach for this feature. There are more decision that can be made around this and improvements. Although, this is the easiest way to implement this since all the environment variables are exposed to all the commands (by using existing code for this) and there are no checkings to be done when running each command.

I QA this under Docker and Local building and my custom env variable was exposed in both scenarios.

Environment variables can be added from the Admin and they will be
used when running build commands. All the variables for that
particular project will be expose to all the commands.
@humitos humitos requested a review from a team November 12, 2018 14:53
@humitos
Copy link
Member Author

humitos commented Nov 12, 2018

If this is the path that we want to follow, I'd add tests for this new code.

@agjohnson
Copy link
Contributor

This doesn't close, #3992. It only wraps up the first phase.

Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Looking good! I think this is close, there were just some implementation details to firm up.

readthedocs/projects/models.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
readthedocs/restapi/views/model_views.py Outdated Show resolved Hide resolved
Add the `environment_variables` field to this serializer that will be
returned only when the user is admin.
@codecov
Copy link

codecov bot commented Nov 13, 2018

Codecov Report

Merging #4894 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4894      +/-   ##
==========================================
+ Coverage    76.6%   76.65%   +0.05%     
==========================================
  Files         158      158              
  Lines       10043    10057      +14     
  Branches     1268     1269       +1     
==========================================
+ Hits         7693     7709      +16     
+ Misses       2008     2007       -1     
+ Partials      342      341       -1
Impacted Files Coverage Δ
readthedocs/doc_builder/environments.py 87.33% <ø> (ø) ⬆️
readthedocs/restapi/serializers.py 97.43% <ø> (-0.1%) ⬇️
readthedocs/projects/models.py 85.44% <100%> (+0.34%) ⬆️
readthedocs/projects/admin.py 91.5% <100%> (+0.42%) ⬆️
readthedocs/projects/tasks.py 71.02% <100%> (+0.46%) ⬆️

@humitos humitos requested a review from a team November 13, 2018 14:34
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

Tests look great! 💯

I'm not sure any of this feedback is necessary, but opened up for discussion at least.

readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/projects/models.py Show resolved Hide resolved
readthedocs/restapi/serializers.py Show resolved Hide resolved
@agjohnson
Copy link
Contributor

🎉

@agjohnson agjohnson merged commit 4fa2746 into master Nov 13, 2018
@agjohnson agjohnson deleted the humitos/builds/expose-env-variables branch November 13, 2018 18:01
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.

2 participants