-
Notifications
You must be signed in to change notification settings - Fork 990
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 #37440 - Enable multiple repositories for host registration #10162
Fixes #37440 - Enable multiple repositories for host registration #10162
Conversation
58ff0ce
to
187cd7e
Compare
One thing that needs improvement is the alignment of the header and the input fields inside the modal. Any hints how to do this best are highly appreciated. |
} | ||
> | ||
<Button | ||
ouiaId="host_reg_add_more_repos" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know I need spell checking for my ouiaIds 🙃
187cd7e
to
8ab2976
Compare
<Split hasGutter> | ||
<Grid hasGutter md={6}> | ||
<FormGroup | ||
label={__('Repository')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MariaAga I had a hard time aligning the labels on the Modal and the text input fields. The corresponding input fields for repos/gpg keys should be aligned with the heading above. Also, I want to place the remove button for one line next to the input fields to the right. Could you give me a hint how to do this best, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok disabling mark_translated now shows me what you see, I'll take a look
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the Split and created a grid which is like
<Grid>
<Item> repo label</item> <Item> GPG label</item> <item>empty space</item>
<Item> input</item> <Item>input</item> <item>remove</item>
</Grid>
And specifically:
https://gist.github.com/MariaAga/0efbad6993ecf112d2d29bab71cf0e4e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and if the model shouldn't grow too much with the screen you can use <Model variant={ModalVariant.small}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot @MariaAga , it looks exactly as I wanted and I finally understood how to make the Grid work!
@@ -72,11 +68,9 @@ Advanced.propTypes = { | |||
handleJwtExpiration: PropTypes.func.isRequired, | |||
handleInvalidField: PropTypes.func.isRequired, | |||
packages: PropTypes.string, | |||
repo: PropTypes.string, | |||
repoGpgKeyUrl: PropTypes.string, | |||
repoData: PropTypes.array.isRequired, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be required field? If yes, should it have some default value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should because I want a pre-defined empty state which I can pass down to all the components. If it is a required prop, it does not need a default prop inside the component.
id="reg_host_repo" | ||
key="reg_host_repo" | ||
value={initRepo} | ||
type="text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it have a check that user adds correct format of the repository? Right now, I can add anything and it accepts it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. We could add something like this later on. A check would require the OS to be set as the format differs for different operating systems. And the OS field is not even mandatory. In the current implementation, you can also enter anything and it would just accept it.
variant="link" | ||
onClick={clearRepositories} | ||
> | ||
{__('Cancel')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suppose I added 3 repositories and open the Modal again, and without editing anything I hit cancel, it removes all the repositories. Shouldn't it keep the previous state if nothing changes? Is it by design?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will fix this, this is not the behavior a user would expect I suppose. I could replace the cancel
Button with a Reset
button and make closing the modal do the same as Confirm
. Cancel
implies that you can discard new changes - this would make the implementation more difficult.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works now as expected.
icon={<PlusCircleIcon />} | ||
onClick={addRepositoryModalInput} | ||
> | ||
{__('Add repository')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, I can add new fields and leave them blank and it doesn't complain or remove them. The set count is correct but the Modal shouldn't have blank rows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addition: This doesn't affect the rendered global_registration.erb
template: If there are 2 repositories and 2 blank lines only to repositories are added during host registration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the empty rows are visible in the UI but they are not sent to the backend. I am unsure what to do about the empty rows in the UI. I think it might confuse the user if I remove them when they click confirm and then re-open the modal for editing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to remove empty rows after submitting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, fixed it.
id="reg_host_gpg_key" | ||
key="reg_host_gpg_key" | ||
value={initGpgKey} | ||
type="text" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is gpg key url a required field or I can also add just repositories and leave the gpg url field blank?
Does it need to have fixed format and should it have check for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The gpg key is not required. If you provide one the repository will be added during registration with gpg_check enabled and the provided gpg_key_url. If not, gpg_check will be disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the check, it is the same as for the repo URL. We could add it later, there is not check in the current implantation, either.
I did some tests for AlmaLinux, SLES and Ubuntu. On my tests the rendered global_registration.erb added the provided repositories as expected (with or without GPG key). |
Thanks a lot @goarsna , @Manisha15 and @MariaAga |
8ab2976
to
a7d74a9
Compare
@stejskalleos Could you re-trigger the tests? Some of them were cancelled for some reason. |
I again had a look at the changes and again did tests (with Ubuntu and AlmaLinux) with adding two repositories during registration. The repositories were added as expected and the repository modal was behaving as expected. => LGTM Regarding the failing test: Could the it be caused by a timeout or hickup as it only says "operating was canceled"? |
a7d74a9
to
9b648e9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good as well thanks!
Some operating systems do not ship all required packages and need additional repositories to make the host registration work. Therefore, users need to provide more than one repository in order to make the host registration work.
In this PR, we replace the existing text fields for the repository and the gpg key that can be entered for host registration with a modal. The modal contains a list where repository entries can be added and removed from.
The API takes a struct array with repository and GPG key URL instead of two separate values per entry. The GPG key URL entry on the UI is optional. If only a GPG key URL is entered, the entry is ignored.