External versions: save state (open / closed) #9128
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
humitos
force-pushed
the
humitos/external-versions-state
branch
4 times, most recently
from
a7f10ff to
5c65f16
Compare
When an external Version is created, we mark it as `.state="open"` and when it's closed/merged we mark is as `.state="closed"`. Note that we cannot use `.active=False` for this as we were doing, because we want to keep serving closed/merged PRs during some extra time after they are closed/merged and El Proxito is not serving inactive versions. With this approach, external versions will be always `.active=True` but with different `.state=`. That state is used by the periodic Celery task to know if the version has to be deleted or not. Closes #9109
humitos
force-pushed
the
humitos/external-versions-state
branch
from
5c65f16 to
8bff524
Compare
humitos
commented
Apr 21, 2022
|
|
||
| Return a JSON response with the following:: | ||
|
|
||
| { | ||
| "version_deactivated": true, | ||
| "closed": true, |
There was a problem hiding this comment.
This is changing the API response. Do we have any concerns about this? In theory, nobody should be listen to this response, tho. We reply this to GitHub / GitLab.
stsewd
reviewed
Apr 21, 2022
Migrate inactive versions to `active` and `state=closed`.
Co-authored-by: Santos Gallegos <[email protected]>
ericholscher
approved these changes
Apr 21, 2022
|
|
||
| Return a JSON response with the following:: | ||
|
|
||
| { | ||
| "version_deactivated": true, | ||
| "closed": true, |
| @@ -51,6 +51,12 @@ | |||
| (EXTERNAL, EXTERNAL_TEXT), | |||
| (UNKNOWN, UNKNOWN_TEXT), | |||
| ) | |||
| EXTERNAL_VERSION_STATE_OPEN = "open" | |||
| EXTERNAL_VERSION_STATE_CLOSED = "closed" | |||
There was a problem hiding this comment.
Do we want more granularity here? Merged vs Closed?
There was a problem hiding this comment.
In the first commit, I used open, closed and merged, but I didn't find how/where we would use it differently, so I removed it because it just complicate the logic. Do you have a good usage for this in mind?
There was a problem hiding this comment.
Mostly displaying the information in the build page, or docs pages. Not a huge deal though, seems fine to start with this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When an external Version is created, we mark it as
.state="open"and when it'sclosed/merged we mark is as
.state="closed".Note that we cannot use
.active=Falsefor this as we were doing, because wewant to keep serving closed/merged PRs during some extra time after they are
closed/merged and El Proxito is not serving inactive versions.
With this approach, external versions will be always
.active=Truebut withdifferent
.state=. That state is used by the periodic Celery task to know ifthe version has to be deleted or not.
Closes #9109