Add publicly visible env vars #7891

flying-sheep · 2021-02-04T10:09:00Z

I have no idea what I’m doing but maybe this eventually fixes #7413

Also IDK how to restructure the API to support this.

Since Versions are always associated with a Project, maybe the better abstraction would be to add a environment_variables property to the Version class. That way we wouldn’t have to do all=version.type != EXTERNAL in two places but could derive it in the property body.

flying-sheep · 2021-02-04T10:31:28Z

Huh, why is the field not found? I’m adding it in the migration!

humitos

Hi, thanks for the PR!

I left some initial comments about this PR. We may also need to update the APIv3 and its documentation to expose .public field

I also think that at some point we will want to decide if the env variable will be available only in PRIVATE or PUBLIC versions.

readthedocs/projects/models.py

flying-sheep · 2021-02-05T18:41:43Z

ha! tests work. why doesn’t the migration check though? the output doesn’t speak to me

stsewd

Thanks for the PR!

So, in the build process we don't have direct access to the db for security reasons. Instead we get the information we need for the build from the API (v2).

As you have already seen, we use a wrapper class for data returned from the API.

readthedocs.org/readthedocs/projects/models.py

Line 1288 in f18c584

class APIProject(Project):

To be able to pass only a subset of env variables into the build process (public env vars),
we need to be able to exclude them from the API first.

stsewd · 2021-02-09T03:44:15Z

ha! tests work. why doesn’t the migration check though? the output doesn’t speak to me

That's because you changed the model but didn't update the migration, you can delete the migration and generate it again to have it match the changes.

readthedocs/projects/models.py

flying-sheep · 2021-02-11T21:58:48Z

OK, done!

Does anything need to be done to have a checkbox to toggle an env var between private and public?

stsewd · 2021-02-15T16:00:07Z

@flying-sheep as mentioned in my comment (#7891 (review)), you'll need to change the API in order to make this work. The privacy level should be returned in the API, so we can filter them from the builders.

readthedocs.org/readthedocs/api/v2/serializers.py

Lines 49 to 53 in 7a117b6

    
           def get_environment_variables(self, obj): 
        
               return { 
        
                   variable.name: variable.value 
        
                   for variable in obj.environmentvariable_set.all() 
        
               }

flying-sheep · 2021-02-16T09:25:09Z

OK!

So the tests pass for ce7dc1f, but if we want db5d9c7, I need help to make the serializer call it with the correct parameter.

Or we could change it so environment_variables is a property again and evaluates to {"VAR_NAME": {"value": "some value", "public": false}}

stsewd · 2021-02-17T22:34:23Z

So the tests pass for ce7dc1f, but if we want db5d9c7, I need help to make the serializer call it with the correct parameter.

Not sure to understand. I think we want to set it to public_only=True by default. The API response is filtered in

readthedocs.org/readthedocs/projects/models.py

Lines 1338 to 1343 in db5d9c7

    
           def environment_variables(self, *, public_only): 
        
               return { 
        
                   name: spec['value'] 
        
                   for name, spec in self._environment_variables.items() 
        
                   if spec['public'] or not public_only 
        
               }

We need to add some tests here for public env vars. And, since the new field is nullable, we need to change the form like this

readthedocs.org/readthedocs/projects/forms.py

Lines 229 to 231 in a3f3705

    
           # Remove the nullable option from the form 
        
           self.fields['analytics_disabled'].widget = forms.CheckboxInput() 
        
           self.fields['analytics_disabled'].empty_value = False

stsewd · 2021-02-17T22:36:23Z

Let me know if you need more help here, thanks for taking this :)

humitos

@flying-sheep This is looking good to me! We are close to have it merged 😄

Let us know if there is something else you need from our side to wrap up this PR.

readthedocs/rtd_tests/tests/test_api.py

flying-sheep · 2021-02-25T10:16:34Z

All done!

flying-sheep · 2021-03-01T12:20:39Z

Ha, got it! The Admin API now still returns all env variables, by calling the method with public_only=False

readthedocs/api/v2/serializers.py

flying-sheep · 2021-03-07T14:02:07Z

All done!

stsewd

Everything looks good to me. I'm not sure how to improve the UI for the detail view, maybe using a read-only form? This is how it looks like right now

readthedocs/doc_builder/python_environments.py

readthedocs/projects/tasks.py

readthedocs/templates/projects/environmentvariable_detail.html

This reverts commit 40fadaa.

stsewd · 2021-03-10T21:22:30Z

Ended up doing this with a readonly form. Not sure about how to handle the value from private env vars. Maybe show **** instead of an empty value? Or maybe remove the field?

flying-sheep · 2021-03-23T13:31:19Z

👋 Could anyone please have a look? My company could really use this.

humitos · 2021-03-23T15:09:16Z

Maybe show **** instead of an empty value? Or maybe remove the field?

@stsewd I think it's better if we show stars so we keep the message "The value isn't show for security reasons".

humitos

I think this is fine.

I'm not sold on showing the value of the environment variable in any case. CircleCI does not show it either. It shows 4 stars and the last 5 chars of the value.

Regarding the UI, we could copy the pattern we have in other places and only have a listing page (without detail page per variable) with the "Delete" button at the right of each element.

The logic is fine to me and I think we can merge this PR. The UI is going to change in the new theme anyways and I think that @agjohnson already built these pages; so worth to ping him here as well.

It seems that some tests are failing. We should take a look at them as well.

humitos · 2021-03-23T15:13:03Z

readthedocs/projects/forms.py

+        # Remove the nullable option from the form
+        self.fields['public'].widget = forms.CheckboxInput()
+        self.fields['public'].empty_value = False


Shouldn't we write a data migration that set public=False to all the existing ones instead?

yeah, but we need to do the migration in the next deploy so we don't have downtime. There are a lot of these missing a migration, may add a PR migrating all of them

readthedocs/projects/forms.py

humitos · 2021-03-23T15:15:47Z

readthedocs/projects/models.py

+    public = models.BooleanField(
+        _('Public'),
+        default=False,
+        null=True,


I think it's better if we write a data migration instead (we can remove the form code as well). This way the data will be valid in our DB.

readthedocs/projects/views/private.py

stsewd · 2021-03-23T16:27:09Z

Regarding the UI, we could copy the pattern we have in other places and only have a listing page (without detail page per variable) with the "Delete" button at the right of each element.

Yeah, that option is good as well, but the value won't be shown, which I think is fine.

stsewd · 2021-03-23T17:13:09Z

I went ahead and deleted the detail view, the delete button is on the listing now.

stsewd · 2021-03-23T17:23:09Z

@humitos let me know what you think now, I can add this to https://github.com/orgs/readthedocs/projects/9 to track the template changes

humitos

@flying-sheep @stsewd looks good to me! 🎉 Thanks! 🙏

@stsewd Please, create a note in the "New theme" project so we can track this.

stsewd · 2021-03-25T17:48:16Z

@flying-sheep thanks for working on this! This should be live by next Tuesday.

flying-sheep · 2021-03-26T12:55:28Z

Thank you for helping me figure it all out! How does the rollout work for rtd.org vs rtd.com? Do they get updated in unison?

stsewd · 2021-03-26T17:32:42Z

Thank you for helping me figure it all out! How does the rollout work for rtd.org vs rtd.com? Do they get updated in unison?

yes, we deploy them the same day, first rtd.org and if everything is good we move to rtd.com.

humitos · 2021-03-30T16:56:00Z

This is already deployed and it's working.

flying-sheep added 3 commits February 4, 2021 11:07

First attempt to add public env vars

ebbd276

Whoops wrong migrations dir

b430b69

Maybe that’s enough for UI?

9efbb59

humitos reviewed Feb 4, 2021

View reviewed changes

readthedocs/projects/models.py Outdated Show resolved Hide resolved

readthedocs/projects/models.py Outdated Show resolved Hide resolved

readthedocs/projects/models.py Outdated Show resolved Hide resolved

flying-sheep added 3 commits February 5, 2021 00:02

Address 2/4 review requests

e7d53ac

whoops

8ceda35

Fix tests

0d5a510

stsewd reviewed Feb 9, 2021

View reviewed changes

readthedocs/projects/models.py Outdated Show resolved Hide resolved

Make nullable

09fb6e0

flying-sheep force-pushed the feature/public-vars branch from 4c9e68e to 09fb6e0 Compare February 11, 2021 21:13

flying-sheep added 2 commits February 16, 2021 10:43

Change API

ce7dc1f

Require specifying publicness

db5d9c7

flying-sheep changed the title ~~First attempt to add public env vars~~ Add publicly visible env vars Feb 16, 2021

humitos reviewed Feb 22, 2021

View reviewed changes

readthedocs/rtd_tests/tests/test_api.py Show resolved Hide resolved

Suggested changes

6c051db

flying-sheep force-pushed the feature/public-vars branch from 177e438 to 6c051db Compare February 25, 2021 10:20

flying-sheep added 2 commits March 1, 2021 12:36

Fix tests

aaea076

Fix tests for real

e1db069

flying-sheep requested a review from stsewd March 1, 2021 12:28

stsewd reviewed Mar 4, 2021

View reviewed changes

readthedocs/api/v2/serializers.py Outdated Show resolved Hide resolved

Change API v2

12d45d3

stsewd reviewed Mar 9, 2021

View reviewed changes

readthedocs/doc_builder/python_environments.py Outdated Show resolved Hide resolved

readthedocs/projects/tasks.py Outdated Show resolved Hide resolved

readthedocs/templates/projects/environmentvariable_detail.html Outdated Show resolved Hide resolved

flying-sheep and others added 5 commits March 9, 2021 20:02

version.is_external

4fce9f6

Improve style

40fadaa

Merge branch 'master' into feature/public-vars

b048282

Revert "Improve style"

0bf14cc

This reverts commit 40fadaa.

Use readonly form to display information

b6345eb

stsewd requested a review from a team March 10, 2021 21:23

humitos approved these changes Mar 23, 2021

View reviewed changes

stsewd added 3 commits March 23, 2021 11:28

Merge branch 'master' into feature/public-vars

af11f5b

Add delete button in listing

a260367

Revert changes in test

d3c7a77

stsewd requested a review from humitos March 23, 2021 17:23

humitos approved these changes Mar 25, 2021

View reviewed changes

stsewd merged commit 9011005 into readthedocs:master Mar 25, 2021

flying-sheep deleted the feature/public-vars branch March 26, 2021 12:53

stsewd mentioned this pull request Apr 5, 2021

Add April newsletter readthedocs/blog#98

Merged

humitos mentioned this pull request May 6, 2021

Docs: update Environment Variables page #8159

Closed

Add publicly visible env vars #7891

Add publicly visible env vars #7891

flying-sheep commented Feb 4, 2021 •

edited

flying-sheep commented Feb 4, 2021

humitos left a comment

flying-sheep commented Feb 5, 2021

stsewd left a comment

stsewd commented Feb 9, 2021

flying-sheep commented Feb 11, 2021 •

edited

stsewd commented Feb 15, 2021

flying-sheep commented Feb 16, 2021 •

edited

stsewd commented Feb 17, 2021

stsewd commented Feb 17, 2021

humitos left a comment

flying-sheep commented Feb 25, 2021

flying-sheep commented Mar 1, 2021

flying-sheep commented Mar 7, 2021

stsewd left a comment

stsewd commented Mar 10, 2021

flying-sheep commented Mar 23, 2021 •

edited

humitos commented Mar 23, 2021

humitos left a comment

humitos Mar 23, 2021

stsewd Mar 23, 2021

humitos Mar 23, 2021

stsewd commented Mar 23, 2021

stsewd commented Mar 23, 2021

stsewd commented Mar 23, 2021

humitos left a comment

stsewd commented Mar 25, 2021

flying-sheep commented Mar 26, 2021

stsewd commented Mar 26, 2021

humitos commented Mar 30, 2021

Add publicly visible env vars #7891

Add publicly visible env vars #7891

Conversation

flying-sheep commented Feb 4, 2021 • edited

flying-sheep commented Feb 4, 2021

humitos left a comment

Choose a reason for hiding this comment

flying-sheep commented Feb 5, 2021

stsewd left a comment

Choose a reason for hiding this comment

stsewd commented Feb 9, 2021

flying-sheep commented Feb 11, 2021 • edited

stsewd commented Feb 15, 2021

flying-sheep commented Feb 16, 2021 • edited

stsewd commented Feb 17, 2021

stsewd commented Feb 17, 2021

humitos left a comment

Choose a reason for hiding this comment

flying-sheep commented Feb 25, 2021

flying-sheep commented Mar 1, 2021

flying-sheep commented Mar 7, 2021

stsewd left a comment

Choose a reason for hiding this comment

stsewd commented Mar 10, 2021

flying-sheep commented Mar 23, 2021 • edited

humitos commented Mar 23, 2021

humitos left a comment

Choose a reason for hiding this comment

humitos Mar 23, 2021

Choose a reason for hiding this comment

stsewd Mar 23, 2021

Choose a reason for hiding this comment

humitos Mar 23, 2021

Choose a reason for hiding this comment

stsewd commented Mar 23, 2021

stsewd commented Mar 23, 2021

stsewd commented Mar 23, 2021

humitos left a comment

Choose a reason for hiding this comment

stsewd commented Mar 25, 2021

flying-sheep commented Mar 26, 2021

stsewd commented Mar 26, 2021

humitos commented Mar 30, 2021

flying-sheep commented Feb 4, 2021 •

edited

flying-sheep commented Feb 11, 2021 •

edited

flying-sheep commented Feb 16, 2021 •

edited

flying-sheep commented Mar 23, 2021 •

edited