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

Refactor (some) class-names to Apt #180

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

m-bucher
Copy link
Contributor

Should fix https://pulp.plan.io/issues/6897

Please comment about possible obstacles.

@pulpbot
Copy link
Member

pulpbot commented Jun 12, 2020

WARNING!!! This PR is not attached to an issue. In most cases this is not advisable. Please see our PR docs for more information about how to attach this PR to an issue.

@m-bucher m-bucher force-pushed the rename_classes_to_apt branch 3 times, most recently from e961965 to d28a89d Compare June 12, 2020 13:12
Copy link
Member

@mdellweg mdellweg left a comment

Choose a reason for hiding this comment

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

I really like it, but I also do see one possible obstacle. Can you please try the migration with a seeded db. I am unsure if existing objects are properly accounted for. You might need to add a handcrafted step.

@quba42
Copy link
Collaborator

quba42 commented Jun 18, 2020

Since the original cause for this change, was inconsistent naming between class names, and API paths, I went and had another look at those.
This change appears to bring them exactly into line with each other. (i.e. All the things that have /apt/ in their API path are now named Apt<entity> for their class.

I wanted to draw attention one more time to the verbatim publisher since it is the other thing with an unusual name:

  • API path /pulp/api/v3/publications/deb/verbatim/, class name VerbatimPublication
  • API path /pulp/api/v3/publications/deb/apt/, class name AptPublication

But this now seems structurally consistent.

Also consistent when compared to pulp_file:

  • API path /pulp/api/v3/publications/file/file/, class name FilePublication

CHANGES/6897.bugfix Outdated Show resolved Hide resolved
@quba42
Copy link
Collaborator

quba42 commented Jun 18, 2020

I really like it, but I also do see one possible obstacle. Can you please try the migration with a seeded db. I am unsure if existing objects are properly accounted for. You might need to add a handcrafted step.

I am not sure if this is a meaningful test, but for what it is worth, I was still able to find everything in my source development box after applying the migration.

How else can I test this?

@quba42 quba42 marked this pull request as ready for review June 18, 2020 16:06
Copy link
Collaborator

@quba42 quba42 left a comment

Choose a reason for hiding this comment

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

As far as I can tell the changelog entry is the only change we need...

@quba42
Copy link
Collaborator

quba42 commented Jun 22, 2020

@m-bucher Do we need to confirm one more time that this really does fix the original issue with the auto generated API bindings?

@m-bucher
Copy link
Contributor Author

@quba42 I will check it out in my forklift.

@m-bucher
Copy link
Contributor Author

m-bucher commented Jun 22, 2020

@quba42 The katello-tests now no longer produce the broken URL, so I guess that is a good sign 😃

@quba42
Copy link
Collaborator

quba42 commented Jun 22, 2020

@mdellweg I will have one more look at a migration tomorrow, and then I would merge this.

@quba42 quba42 merged commit b94953d into pulp:master Jun 23, 2020
@quba42 quba42 deleted the rename_classes_to_apt branch June 23, 2020 07:50
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