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

Fixes #25980: Update tags whitelist only for docker repos #7962

Merged
merged 1 commit into from Feb 21, 2019

Conversation

@akofink
Copy link
Member

commented Feb 4, 2019

To test:

  1. Create a non-docker repo, verify that docker_tags_whitelist is nil
  2. Edit this repository using web UI (e.g. change name)
  3. Verify that docker_tags_whitelist is nil (and not [] or anything else)
@theforeman-bot

This comment has been minimized.

Copy link

commented Feb 4, 2019

Issues: #25980

@parthaa

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Hmm so I noticed this discrepancy. Is this intentional ?

$ hammer repository create --name smart --content-type=docker --product-id=270 --url=http://index.docker.io  --upstream-name=alpine

$ curl -u admin:changeme https://gamma.partello.example.com/katello/api/repositories/79| jq . |grep docker_tags_whitelist
  "docker_tags_whitelist": nil,

Go to the UI and edit this repo change name to something else

$ curl -u admin:changeme https://gamma.partello.example.com/katello/api/repositories/79| jq . |grep docker_tags_whitelist
  "docker_tags_whitelist": [],

notice how nil became [] . I am ok with keeping it [] but I think @mirzal had issues with it. I would rather say lets be consistent and always set white_tags list to []

@mirzal

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

@parthaa yeah, I noticed that, too. In hammer, fresh docker repo will not display "Tags whitelist" field at all. After changing anything, it will. The same effect can be achieved by setting tags whitelist and then removing it.

True, having this inconsistency is less than perfect, but I'm willing to let it go. It might raise eyebrow or two, but doesn't have any significant impact anywhere. If it's easy to fix, sure, it would be nice. Otherwise, let's not stress too much over it.

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

I don't think it'd be too difficult to set [] on create for the empty state.

@akofink akofink force-pushed the akofink:25980 branch from e49e6bb to 710368c Feb 7, 2019

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 7, 2019

Updated - New non-docker repos should have a nil docker_tags_whitelist value on create or update. Docker repos should never have nil - it should at least be [] on create or update.

@akofink akofink force-pushed the akofink:25980 branch from 710368c to 2495fa9 Feb 7, 2019

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

The test errors don't seem related to this change. 🤷‍♂

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 12, 2019

Tests are green. @parthaa did you want to block this PR on refactoring the repositories_controller.rb?

@akofink akofink force-pushed the akofink:25980 branch from 2495fa9 to 1a240a5 Feb 18, 2019

@parthaa

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Tests are green. @parthaa did you want to block this PR on refactoring the repositories_controller.rb?

I m ok with this . I 'd just recommend tinkering/removing https://github.com/Katello/katello/blob/master/app/models/katello/root_repository.rb#L186-L187

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 18, 2019

Updated to remove the unnecessary root_repository code

@akofink akofink force-pushed the akofink:25980 branch from 1a240a5 to fd2a215 Feb 21, 2019

@akofink

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

There was a rubocop issue. It should pass this time! 🤞

@akofink akofink merged commit 098bc25 into Katello:master Feb 21, 2019

2 checks passed

katello Build finished. 4173 tests run, 9 skipped, 0 failed.
Details
prprocessor Commit message style is correct
Details

@akofink akofink deleted the akofink:25980 branch Feb 21, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.