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

Renaming base fields #218

Merged
merged 2 commits into from Oct 9, 2019
Merged

Renaming base fields #218

merged 2 commits into from Oct 9, 2019

Conversation

fao89
Copy link
Member

@fao89 fao89 commented Oct 2, 2019

_id to pulp_id
_created to pulp_created
_last_updated to pulp_last_updated
_href to pulp_href

ref #5457
https://pulp.plan.io/issues/5457

Required PR: pulp/pulpcore#317
Required PR: pulp/pulpcore-plugin#137
Required PR: pulp/pulp-smash#1220

@fao89 fao89 requested a review from a team October 2, 2019 20:39
@pep8speaks
Copy link

pep8speaks commented Oct 2, 2019

Hello @fabricio-aguiar! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-10-09 18:48:06 UTC

model_name='tag',
old_name='_id',
new_name='pulp_id',
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you update this migration instead of adding this to 0013_pulp_fields.py?

Copy link
Member Author

Choose a reason for hiding this comment

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

@@ -18,7 +18,7 @@
coalesce(string_agg("ansible_tag"."name", ' '), '')
), 'B')
FROM
"ansible_tag" INNER JOIN "ansible_collectionversion_tags" ON ("ansible_tag"."_id" = "ansible_collectionversion_tags"."tag_id")
"ansible_tag" INNER JOIN "ansible_collectionversion_tags" ON ("ansible_tag"."pulp_id" = "ansible_collectionversion_tags"."tag_id")
Copy link
Member Author

Choose a reason for hiding this comment

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

@daviddavis I didn't know how to solve it without changing this migration

Copy link
Contributor

@daviddavis daviddavis Oct 3, 2019

Choose a reason for hiding this comment

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

I think we probably have to drop the vector, rename the field, and then recreate the vector. cc @bmbouter

Copy link
Member Author

Choose a reason for hiding this comment

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

how can I drop it?

Copy link
Member

Choose a reason for hiding this comment

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

Given this is still pre-GA, and the complexity of having to "fix" the migration with hand-generated code in a new migration I'm ok with modifying this migration in-place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either squashing or updating the migration is cool with me.

@@ -0,0 +1 @@
Change `_id`, `_created`, `_last_updated`, `_href` to `pulp_id`, `pulp_created`, `pulp_last_updated`, `pulp_href`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be a .removal

_id to pulp_id
_created to pulp_created
_last_updated to pulp_last_updated
_href to pulp_href

ref #5457
https://pulp.plan.io/issues/5457
@fao89 fao89 merged commit 078942a into pulp:master Oct 9, 2019
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.

None yet

4 participants