-
Notifications
You must be signed in to change notification settings - Fork 223
Add AppStreams to SSM #11135
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
base: master
Are you sure you want to change the base?
Add AppStreams to SSM #11135
Conversation
|
👋 Hello! Thanks for contributing to our project. You can see the progress at the end of this page and at https://github.com/uyuni-project/uyuni/pull/11135/checks If you are unsure the failing tests are related to your code, you can check the "reference jobs". These are jobs that run on a scheduled time with code from master. If they fail for the same reason as your build, it means the tests or the infrastructure are broken. If they do not fail, but yours do, it means it is related to your code. Reference tests: KNOWN ISSUES Sometimes the build can fail when pulling new jar files from download.opensuse.org . This is a known limitation. Given this happens rarely, when it does, all you need to do is rerun the test. Sorry for the inconvenience. For more tips on troubleshooting, see the troubleshooting guide. Happy hacking! |
4006ebd to
31f057b
Compare
web/html/src/manager/systems/appstreams/ssm-appstreams-configure.tsx
Outdated
Show resolved
Hide resolved
web/html/src/manager/systems/appstreams/ssm-appstreams-channel-selection.tsx
Outdated
Show resolved
Hide resolved
web/html/src/manager/systems/appstreams/ssm-appstreams-channel-selection.tsx
Outdated
Show resolved
Hide resolved
web/html/src/manager/systems/appstreams/ssm-appstreams-configure-list.tsx
Outdated
Show resolved
Hide resolved
2e3c3fa to
1f40aea
Compare
| <> | ||
| {row.parentId && ( | ||
| <img | ||
| style={{ marginLeft: "4px", marginRight: "8px", verticalAlign: "middle" }} |
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.
Instead of inline styles, it would be preferable to use a css module, see e.g. web/html/src/components/table/SearchPanel.module.scss and web/html/src/components/table/SearchPanel.tsx.
Etheryte
left a comment
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.
Thank you very much for the updates, LGTM. 👍
1f40aea to
bfab440
Compare
bfab440 to
b57d0bf
Compare
cbbayburt
left a comment
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 left a couple of questions below to clarify, but overall it looks good to me 👍
One thing though; not strictly necessary, but since we're not in a rush, could you add a shortcut link to the SSM Overview tab like the others?
java/code/src/com/suse/manager/webui/controllers/appstreams/AppStreamsController.java
Outdated
Show resolved
Hide resolved
java/code/src/com/suse/manager/webui/controllers/appstreams/AppStreamsController.java
Outdated
Show resolved
Hide resolved
cbbayburt
left a comment
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.
Meanwhile, I deployed the code and tested it manually. It works great!
I only have one more request below regarding RBAC.
| INSERT INTO access.namespace (namespace, access_mode, description) | ||
| VALUES ('systems.appstreams', 'W', 'Manage AppStreams on systems (SSM)') | ||
| ON CONFLICT (namespace, access_mode) DO NOTHING; |
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.
Since we're going to use the existing systems.software.appstreams namespace, you don't need to insert anything to the namespace table.
| INSERT INTO access.namespace (namespace, access_mode, description) | |
| VALUES ('systems.appstreams', 'W', 'Manage AppStreams on systems (SSM)') | |
| ON CONFLICT (namespace, access_mode) DO NOTHING; |
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.
BTW, if we were going to use a namespace, you would need to add a row into the accessgroupnamespace table for each access group that should have access to the new namespace by default.
If you don't add the namespace to any access group, effectively, only Uyuni admin (not even org admin) can access the namespace.
You don't need to do this for existing namespaces, unless you want to change the default access rules for them for some reason.
- Update migration script to use the existing 'systems.software.appstreams' RBAC namespace instead of creating a new one. - Fix typo in the endpoint path within the migration script. - Remove unnecessary DISTINCT clause from LIST_SSM_SERVERS_IN_CHANNEL_SQL query. - Rename 'getAppStreamChannelFromPath' to 'resolveAppStreamChannel'. - Change invalid channel handling to use 'response.redirect' instead of a server-side forward, ensuring the URL matches the view. - Add a shortcut link to the SSM Overview tab.
b57d0bf to
781ca87
Compare
|
Thanks for addressing my comments, @wweellddeerr. I have just one final suggestion: For RBAC, we have this new ACL called With this ACL, we can let the AppStreams tab render only if the user has access to Here's some examples: SSM tabs:uyuni/java/code/webapp/WEB-INF/nav/ssm.xml Lines 27 to 29 in 8c6139b
SSM index page shortcuts:uyuni/java/code/webapp/WEB-INF/pages/ssm/ssmindex.jsp Lines 68 to 80 in 8c6139b
|
What does this PR change?
This PR introduces AppStreams configuration feature to SSM through WebUI and API.
Codespace
Check if you already have a running container clicking on
GUI diff
After:
Documentation
No documentation needed: add explanation. This can't be used if there is a GUI diff
No documentation needed: only internal and user invisible changes
Documentation issue was created: Link for SUSE Multi-Linux Manager contributors, Link for community contributors.
API documentation added: please review the Wiki page Writing Documentation for the API if you have any changes to API documentation.
(OPTIONAL) Documentation PR
DONE
Test coverage
ℹ️ If a major new functionality is added, it is strongly recommended that tests for the new functionality are added to the Cucumber test suite
No tests: add explanation
No tests: already covered
Unit tests were added
Cucumber tests were added
DONE
Links
Issue(s): https://github.com/SUSE/spacewalk/issues/24196
Changelogs
Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository
If you don't need a changelog check, please mark this checkbox:
If you uncheck the checkbox after the PR is created, you will need to re-run
changelog_test(see below)Re-run a test
If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:
Before you merge
Check How to branch and merge properly!