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

Design doc for changing version's slug #6273

Closed

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Oct 10, 2019

Related to #6204 #4505 #5318

@stsewd stsewd requested a review from a team October 10, 2019 18:49
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.

The document looks good.

I'm not approving it yet because I want to think a little more about this and read other people's thoughts as well.

For now, I'm mentioning a problem in the solution proposed when changing the slug: delete and re-build.

Copy link
Member

@ericholscher ericholscher left a comment

Choose a reason for hiding this comment

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

This feels really complex, which is the reason I have wanted to not allow this in the past. I don't think we should build an incredibly complex system for handling renaming of the slug. We should either let users shoot themselves in the foot, or not allow it. I tend to err on the side of not allowing it, and having them ask us when they want it.

This is the same conclusion we came to last time, and I still haven't heard a good reason for why we should allow users to rename things instead of just emailing support.

We need to check that the new slug is compatible with our current code.

We can't allow users to change the slug of machine created versions,
because those have a special meaning in our code (like ``stable`` and ``latest``).
Copy link
Member

Choose a reason for hiding this comment

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

What about if a user pushes a branch named latest and then changes the slug of 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.

We already allow users to create a latest branch that isn't machine created, the behavior is like any other branch.

def test_machine_attr_when_user_define_latest_tag_and_delete_it(self):
"""The user creates a tag named ``latest`` on an existing repo, when
syncing the versions, the RTD's ``latest`` is lost (set to
machine=False) and doesn't update automatically anymore, when the tag is
deleted on the user repository, the RTD's ``latest`` is back (set to
machine=True)."""
version_post_data = {
'branches': [
{
'identifier': 'origin/master',
'verbose_name': 'master',
},
],
'tags': [
# User new stable
{
'identifier': '1abc2def3',
'verbose_name': 'latest',
},
],
}
resp = self.client.post(
reverse('project-sync-versions', args=[self.pip.pk]),
data=json.dumps(version_post_data),
content_type='application/json',
)
self.assertEqual(resp.status_code, 200)
# The tag is the new latest
version_latest = self.pip.versions.get(slug='latest')
self.assertEqual(
'1abc2def3',
version_latest.identifier,
)
# Deleting the tag should return the RTD's latest
version_post_data = {
'branches': [
{
'identifier': 'origin/master',
'verbose_name': 'master',
},
],
'tags': [],
}
resp = self.client.post(
reverse('project-sync-versions', args=[self.pip.pk]),
data=json.dumps(version_post_data),
content_type='application/json',
)
self.assertEqual(resp.status_code, 200)
# The latest isn't stuck with the previous commit
version_latest = self.pip.versions.get(slug='latest')
self.assertEqual(
'master',
version_latest.identifier,
)
self.assertTrue(version_latest.machine)

Copy link
Member

Choose a reason for hiding this comment

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

Right, but won't that break places where we expect a latest slug on a project?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, the only parts where this has a special meaning is when we do a sync for versions and when sorting. So it would be sorted first (like a machine created version), but probably that's what users want.

@agjohnson
Copy link
Contributor

As you mentioned, does the usage of an alias resolve some of the problems we have here, or make them easier? We have a lot of internal mechanisms, especially around syncing versions/etc, that use the slug or verbose_name. Would having a separate alias attribute for just the operations that the user cares about (serving doc version, footer version list, etc) allow us to keep existing version logic and only alter where the user facing version name is used?

I don't have a clear idea of all the places in code that we're using version objects right now.

@stsewd
Copy link
Member Author

stsewd commented Oct 16, 2019

As you mentioned, does the usage of an alias resolve some of the problems we have here, or make them easier?

It solves the problem with having the docs offline for a time, yes (there is a solution for this in the design doc too). But it introduces more changes, we would need to check the current logic where we use the slug and use the alias (or/and the slug) instead. Also, using an alias doesn't solve the problem to show a different name on the footer or change the order the version is sorted, if we want to solve that with the alias, we'll need to change more code.

Also, the only special cases for the slug are latest and stable, which we already support to be created by a user.

@stale
Copy link

stale bot commented Nov 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 Nov 30, 2019
@stsewd stsewd added the Needed: design decision A core team decision is required label Dec 2, 2019
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Dec 2, 2019
@humitos humitos mentioned this pull request Aug 28, 2023
@humitos
Copy link
Member

humitos commented Dec 2, 2024

Closing in favor of https://github.com/readthedocs/meta/discussions/147

@humitos humitos closed this Dec 2, 2024
@stsewd stsewd deleted the design-doc-change-version-slug branch January 27, 2025 16:26
stsewd added a commit that referenced this pull request Feb 4, 2025
Basically what I had in my old design doc
#6273

- When changing the slug, we check that it's a valid one (lowercase,
only number, etc), in case the slug is not valid, we suggest a valid
slug based on the invalid one. Why not just transform the value to a
valid slug? Changing the slug is a breaking operation, we want to avoid
surprises for the user, if they set a slug to "foo/bar", and then the
actual slug is "foo-bar", that's unexpected.
- `project` is used as s hidden field, following the same pattern we
already have for other forms (this way all normal validations like the
unique constraint work instead of getting a 500).
- Editing the slug is disabled for machine created versions (stable,
latest).
- I'm not restricting the usage of latest or stable, since those will be
non-machine created, similar to what a user does already if they name a
version stable/latest.
- If a version was active and its slug was changed, the resources
attached to the old slug are removed before triggering a new build.
- Cache is always purged after deleting resources from the version.

I was playing with using the "pattern" attribute for the input, so it
validates the slug before submitting the form.


![vlcsnap-2025-01-28-12h30m33s792](https://github.com/user-attachments/assets/1b89a0fc-74e4-4f89-ab37-7498b55708f4)


But I think showing the error with the suggested slug works better,
maybe we can port that to JS instead as a future improvement.

![Screenshot 2025-01-28 at 12-30-10 rynoh-pdf - test-builds - Read the
Docs](https://github.com/user-attachments/assets/a38bb6e8-295c-457c-9a77-702b574d082d)


### Decisions

- Expose this as a feature flag first, or just document it right away?

### TODO

- Allow changing the slug using the API.
- Suggest a redirect from the old slug to the new one.

The idea implemented here comes from this comment:
readthedocs/meta#147 (reply in thread)

ref readthedocs/meta#147
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needed: design decision A core team decision is required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants