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
feat(volumes): add browse volume button to other related views #3089
base: develop
Are you sure you want to change the base?
feat(volumes): add browse volume button to other related views #3089
Conversation
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.
Functionally this looks 100% good to me and is ready for technical view.
cc @xAt0mZ for technical review
@deviantony as a note: with the disable volume browse toggle
introduced in 1.22.1, we need to make sure that volume browse is correctly restricted in the new areas this PR introduces
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.
LGTM
Don't merge as it can require some changes for the point mentionned above
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.
@diegovilar are you able to rebase this to version 1.22.1? I will need to test this after rebasing to make sure that it does not conflict with this volume browse toggle feature introduced in Portainer version 1.22.1
50549d4
to
86cb2b8
Compare
@itsconquest, I've rebased it (current master) and made the necessary changes to work with this volume browse toggle feature. Please re-review the PR. Also, CodeClimate is complaining that the code used to check if the user can browse is found in 3 different files. Since the code requires access to the services ExtensionService, Authentication and ApplicationState, all of which need to be injected by Angular, I guess the correct way would be to extract it to a utility service, or to a simple utility method that requires those services as parameters. If you guys want that issue handled, please let me know how to proceed. |
@diegovilar don't worry about codeclimate. cc @itsconquest for functional review first |
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.
@diegovilar Apologies for the late update on this, things have been crazy leading up to the 2.0 release.
I've just tested this with RBAC and functionally this looked OK, with one minor thing that I think needs changing:
For RBAC users logged in (when browsing is disabled), the browse button is shown (as disabled) in the container details and volume details views, rather than being hidden as we currently do for the button in the volumes view. This will need to be changed so that the button is not shown in these views when the setting is disabled.
This PR will also require another rebase as the develop branch has now gotten ahead.
If you are not able to contribute further on this PR (due to the time it has taken us to move ahead on it) I can rebase and make the require changes on your behalf :)
Closes #2265
Added to the following views the same "browse" button used by the
volumesDatatable
component that allows the user to browse files in a volume:Volume details view
Container details view
The buttons use the authorization
DockerAgentBrowseList
and are only shown when these both expressions are true:!EndpointProvider.offlineMode()
StateManager.getState().endpoint.mode.agentProxy