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

Cloud images - support duplicated names #3467

Merged
merged 1 commit into from
Jun 25, 2019

Conversation

tomez
Copy link
Contributor

@tomez tomez commented Jun 25, 2019

No description provided.

@coveralls
Copy link

coveralls commented Jun 25, 2019

Coverage Status

Coverage decreased (-0.0009%) to 84.554% when pulling faf0b80 on tomez:feature/cloud-images-duplicate-names into bf592a3 on allegro:ng.

Copy link
Contributor

@romcheg romcheg left a comment

Choose a reason for hiding this comment

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

Left several comments inline.

@@ -514,7 +514,7 @@ class CloudProviderAdmin(RalphAdmin):
@register(CloudImage)
class CloudImageAdmin(RalphAdmin):
list_display = ['name', 'image_id']
list_filter = ['name', 'image_id']
list_filter = ['image_id']
Copy link
Contributor

Choose a reason for hiding this comment

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

After giving it a thought I think it's actually better to leave the name here and get rid of the uuid -- I can imagine someone recalling a part of the image name and trying to filter by that, however, remembering an image uuid is unlikely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another point is that it would be useful to enable API filtering by both name and uuid fields. API filters are defined on view sets and by default are copied from admin. So in this case it's necessary to add a list_filter property to CloudImageViewSet. Here you can find an example: https://github.com/allegro/ralph/blob/ng/src/ralph/virtual/api.py#L181

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -16,13 +16,9 @@ class Migration(migrations.Migration):
name='CloudImage',
Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot change already merged migrations -- they are already applied somewhere. Just generate a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tomez tomez force-pushed the feature/cloud-images-duplicate-names branch 2 times, most recently from 1c0a169 to a681457 Compare June 25, 2019 10:40
@tomez tomez force-pushed the feature/cloud-images-duplicate-names branch from a681457 to faf0b80 Compare June 25, 2019 10:41
Copy link
Contributor

@romcheg romcheg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@romcheg romcheg merged commit 8dc8ce3 into allegro:ng Jun 25, 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