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

[Signing 2] Add only signed options to remote form #1625

Merged
merged 1 commit into from
Mar 24, 2022
Merged

[Signing 2] Add only signed options to remote form #1625

merged 1 commit into from
Mar 24, 2022

Conversation

brumik
Copy link
Contributor

@brumik brumik commented Feb 9, 2022

  • There is maybe a need to add a sign that repo is syncs only signed only

Screenshot from 2022-02-09 09-46-40

@brumik brumik requested a review from himdel February 9, 2022 08:47
@brumik brumik mentioned this pull request Feb 9, 2022
2 tasks
@ZitaNemeckova
Copy link
Member

ZitaNemeckova commented Mar 2, 2022

@brumik Is this ready for review?

Api does not supports it yet, by chat conversation we will not implement this now.

Maybe this needs to be better marked "to not be reviewed" :) WIP in title or making it a draft.

@brumik
Copy link
Contributor Author

brumik commented Mar 2, 2022

@ZitaNemeckova this PR is ready. That particular feature can come later when api supports it.

@ZitaNemeckova
Copy link
Member

@brumik Right now I can say it looks good code-wise 👍

BUT it's impossible to test without API and definetely shouldn't be merged because it breaks the form rigth now if an user changes the value of signed_only. So that's why I would add some kind of warning to prevent anyone from merging it before API.

@brumik brumik changed the title [Signing 2] Add only signed options to remote form [Signing 2] Add only signed options to remote form [WAIT FOR API] Mar 2, 2022
@brumik brumik changed the title [Signing 2] Add only signed options to remote form [WAIT FOR API] [Signing 2] Add only signed options to remote form Mar 3, 2022
Copy link
Collaborator

@himdel himdel left a comment

Choose a reason for hiding this comment

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

LGTM :)

Api does not supports it yet, by chat conversation we will not implement this now.

Is this still relevant?

@brumik
Copy link
Contributor Author

brumik commented Mar 24, 2022

@himdel Yes, we will card it up probably now.

@brumik brumik merged commit 03d148f into ansible:master Mar 24, 2022
@brumik brumik deleted the remote-form-sync-only-signed branch March 24, 2022 17:17
@himdel
Copy link
Collaborator

himdel commented Apr 5, 2022

Yes, we will card it up probably now.

What does this mean?

That sounds like "yes, we're not doing this" but if so, why the merge?


As per Bruno: API supports and exposes that field, we must be able to set a remote to sync only signed content

So sounds like this should be merged 👍, treating as done.

@brumik when you're back, please clarify if there are any missing parts, or if you still have any concerns regarding API support.


EDIT: the missing parts are probably in a separate issue now - https://issues.redhat.com/browse/AAH-1476

himdel added a commit to himdel/ansible-hub-ui that referenced this pull request Apr 7, 2022
…, not registries

Follow-up to ansible#1625

the new switch was also visible for remoteType=registry, hiding

No-Issue
himdel added a commit that referenced this pull request Apr 7, 2022
…, not registries (#1864)

Follow-up to #1625

the new switch was also visible for remoteType=registry, hiding

No-Issue
himdel pushed a commit to RedHatInsights/ansible-hub-ui-build that referenced this pull request Apr 7, 2022
…, not registries (#1864)

Follow-up to ansible/ansible-hub-ui#1625

the new switch was also visible for remoteType=registry, hiding

No-Issue
himdel pushed a commit to RedHatInsights/ansible-hub-ui-build that referenced this pull request Apr 7, 2022
…, not registries (#1864)

Follow-up to ansible/ansible-hub-ui#1625

the new switch was also visible for remoteType=registry, hiding

No-Issue
@brumik
Copy link
Contributor Author

brumik commented Apr 11, 2022

Thanks for the merge, yes, the missing part is moved to a separate issue.

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

3 participants