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 #35026 - As a user, I can edit ACS on the UI #10184

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Jul 5, 2022

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Screenshot from 2022-07-29 09-21-58

@theforeman-bot
Copy link

Issues: #35026

@ianballou
Copy link
Member

Some comments about smart proxies and products with respect to organizations:

We'll need to remove the filtering on smart proxies and products since organization scope doesn't apply here.

Smart proxy names can still be used, but product names cannot because product names aren't unique across organizations.

@ianballou
Copy link
Member

Also, it would be good to add a blurb telling folks that ACSs affect all repository syncs on any particular smart proxy.

@sjha4
Copy link
Member Author

sjha4 commented Jul 12, 2022

Some comments about smart proxies and products with respect to organizations:

We'll need to remove the filtering on smart proxies and products since organization scope doesn't apply here.

Smart proxy names can still be used, but product names cannot because product names aren't unique across organizations.

I will get the simplified ACS create and view PR in so we can get edits to work for both in this PR.

@sjha4
Copy link
Member Author

sjha4 commented Jul 29, 2022

Also, it would be good to add a blurb telling folks that ACSs affect all repository syncs on any particular smart proxy.

Ian, can you give this a test now and also where do you think this notice is appropriate? On the create wizard > Smart proxy screen + Review screen and the edit smart proxy modal?

To-do: Tests

@ianballou
Copy link
Member

where do you think this notice is appropriate? On the create wizard > Smart proxy screen + Review screen and the edit smart proxy modal?

That sounds perfect to me!

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

One thing I noticed was how the edit wizard handled errors. The error popped up in the background and the wizard stayed open. I tried adding a product to an yum ACS that had only a a file repo in it.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

I realized that clearing the upstream username and password isn't working. It seems we need something like this for ACS: https://github.com/Katello/katello/blob/master/app/models/katello/root_repository.rb#L263

@sjha4 sjha4 force-pushed the update_acs_details branch 3 times, most recently from b00e9e6 to 24b0cbd Compare August 1, 2022 17:28
Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Everything is looking good to me now! Let me know when the last tests make it in.

@sjha4
Copy link
Member Author

sjha4 commented Aug 2, 2022

@ianballou : Added some more tests for simplified acs edits. Should be ready for re-review.

@sjha4 sjha4 merged commit bfb0b08 into Katello:master Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants