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

Version Model Update for Pull Request Builder #5750

Closed
wants to merge 31 commits into from

Conversation

saadmk11
Copy link
Member

@saadmk11 saadmk11 commented May 31, 2019

This PR will Update the Version Model to be used for Pull Request Builder.

Tasks:

  • Add pull request VERSION_TYPES
  • Update Version Model Methods
  • Add Version Model Manager for internal and external Versions
  • Add tests for Querysets

We need to think about how we want to create slugs for PR Versions. Should the slug be like this
pr-<number> or only <pr_number>. We have to think about how we can make pr version slugs unique so that it doesn't cause problems for the normal Versions. if we do pr-<number> the vcs_url() method of the Version Model needs to be updated.

Feel free to let me know if there are other places that we need to update for this PR.

I would like to get some initial feedback's to continue the work on slug creation and writing tests.

@saadmk11 saadmk11 added the PR: work in progress Pull request is not ready for full review label May 31, 2019
@saadmk11 saadmk11 closed this May 31, 2019
@saadmk11 saadmk11 reopened this May 31, 2019
@safwanrahman safwanrahman self-requested a review June 1, 2019 02:06
@saadmk11 saadmk11 requested review from ericholscher and a team June 3, 2019 09:54
@stsewd
Copy link
Member

stsewd commented Jun 3, 2019

We may want to use managers or another pattern, feels like a lot of places where we can forget to exclude PRs from the actual versions

@saadmk11 saadmk11 force-pushed the version-update branch 2 times, most recently from 4b64d7d to f8ecdb7 Compare June 4, 2019 08:43
@@ -130,7 +130,7 @@ def active_versions(self, request, **kwargs):
Project.objects.api(request.user),
pk=kwargs['pk'],
)
versions = project.versions.filter(active=True)
versions = project.versions(manager='internal').filter(active=True)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the normal way to handle this? Is it not normally project.versions.internal.filter?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes this is the normal way to do this if we use multiple managers.
ref: Dajngo Docs

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.

Looks good overall!

build_pk=build.pk,
version_pk=version.pk,
)
elif version == 'internal':
Copy link
Member

Choose a reason for hiding this comment

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

These options should be documented somewhere. Also we should be using constants for these, not strings INTERNAL = 'internal' in projects/constants.py or similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay I'll Add them in the constants. Do we Document Management Commands Somewhere?

Copy link
Member

@ericholscher ericholscher Jun 5, 2019

Choose a reason for hiding this comment

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

Not currently -- perhaps we should do something like this: https://docs.readthedocs.io/en/latest/settings.html

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay i'll Add these in another PR.

@saadmk11 saadmk11 closed this Jun 6, 2019
@saadmk11 saadmk11 reopened this Jun 6, 2019
@saadmk11 saadmk11 removed the PR: work in progress Pull request is not ready for full review label Jun 6, 2019
@saadmk11 saadmk11 requested review from ericholscher and a team June 6, 2019 18:31
@saadmk11 saadmk11 closed this Jun 11, 2019
@saadmk11 saadmk11 reopened this Jun 11, 2019
@stsewd
Copy link
Member

stsewd commented Jun 18, 2019

We should check that Version.internal.public and Version.external.public is doing the filter correctly, because public can add all the versions ignoring any previous filter (see #5783)

@stsewd
Copy link
Member

stsewd commented Jun 18, 2019

About the slug, doesn't matter what we choose, the slug is unique. We add prefixes to make a slug unique if there is one with that slug.

User creates a branch pr-200, the slug is created as pr-200, if a pull requests #200 is created the slug is pr-200_a.

@saadmk11
Copy link
Member Author

saadmk11 commented Jun 18, 2019

We should check that Version.internal.public and Version.external.public is doing the filter correctly, because public can add all the versions ignoring any previous filter (see #5783)

@stsewd I have already added some tests. I'll add some more to be more careful.

About the slug, doesn't matter what we choose, the slug is unique. We add prefixes to make a slug unique if there is one with that slug.

User creates a branch pr-200, the slug is created as pr-200, if a pull requests #200 is created the slug is pr-200_a.

Then we have to add the prefix (pr-) for the External version slugs. how do you think we should handle vcs_url() ?

@stsewd
Copy link
Member

stsewd commented Jun 18, 2019

In the version model we have identifier and verbose_name, I guess we are going to add the commit hash in identifier, and we can add the pr indentifier in verbose_name, this should be 123 (not pr-123) for github or anything that is used by the provider to identify a pr. That way is more easy to build the url

@ericholscher
Copy link
Member

Closing this in favor of #5823, which should now be our feature branch for this work until it's merged into master.

@saadmk11 saadmk11 deleted the version-update branch June 19, 2019 06:57
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.

4 participants