Fixes #37440 - Enable multiple repositories for host registration #10162
Conversation
nadjaheitmann
force-pushed
the
37440_multiple_repos_on_host_register
branch
from
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.
I did not know I need spell checking for my ouiaIds 🙃
nadjaheitmann
force-pushed
the
37440_multiple_repos_on_host_register
branch
from
187cd7e
to
8ab2976
Compare
| <Split hasGutter> | ||
| <Grid hasGutter md={6}> | ||
| <FormGroup | ||
| label={__('Repository')} |
There was a problem hiding this comment.
@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.
Could you attach a simple sketch of what you mean? I see the model as you describe you want it to be
There was a problem hiding this comment.
In your screen shot it looks like it should. For me, it looks like this:
The headings are not aligned with the input fields.
There was a problem hiding this comment.
Ok disabling mark_translated now shows me what you see, I'll take a look
There was a problem hiding this comment.
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.
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.
Thanks a lot @MariaAga , it looks exactly as I wanted and I finally understood how to make the Grid work!
nadjaheitmann
changed the title
| @@ -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.
Should it be required field? If yes, should it have some default value?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
| icon={<PlusCircleIcon />} | ||
| onClick={addRepositoryModalInput} | ||
| > | ||
| {__('Add repository')} |
There was a problem hiding this comment.
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.
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.
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.
I think it makes sense to remove empty rows after submitting
| id="reg_host_gpg_key" | ||
| key="reg_host_gpg_key" | ||
| value={initGpgKey} | ||
| type="text" |
There was a problem hiding this comment.
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.
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.
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 |
nadjaheitmann
force-pushed
the
37440_multiple_repos_on_host_register
branch
from
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"? |
nadjaheitmann
force-pushed
the
37440_multiple_repos_on_host_register
branch
from
a7d74a9
to
9b648e9
Compare
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.