-
Notifications
You must be signed in to change notification settings - Fork 15
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
Handle linking to unregistered hosts #1560
Conversation
a3df8ec
to
7b77fb3
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.
For the record, knowing that the PR is in draft.
This change will cause massive conflicts on main, as this file is even deleted.
I encourage to base this work on main
branch.
26ea678
to
d44dd39
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.
@rtorrero I think we could put this logic in the already existing HostLink
component, or create a new one, as we need to have this logic in the ascs/ers clusters as well.
What do you think?
20e91cd
to
5041a67
Compare
I had this conversation a few weeks with @dottorblaster and we went for the current approach to avoid adding artifacts to places where But your point avoid avoiding duplicating the logic in the ascs/ers clusters is a valid concern. As you suggested in Slack, creating a separate |
b145b18
to
f2a6a28
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.
Looking good!
Some minor changes and we are ready to merge
@jagabomb could you have a look on how it looks?
Changes available in PR storybook. Stories AscsErsClusterDetails
and HanaClusterDetails
...Single.args, | ||
hosts: [buildHostsFromAscsErsClusterDetails(details).pop()], | ||
}, | ||
render: (args) => ( |
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.
we need to at some point move this render
to the main export default
to avoid not needed repetition.
Could we do it in a next PR?
It would we as simple as moving this exact render
up
assets/js/components/ClusterDetails/AscsErsClusterDetails.stories.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/AscsErsClusterDetails.test.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/HanaClusterDetails.stories.jsx
Outdated
Show resolved
Hide resolved
assets/js/components/ClusterDetails/HanaClusterDetails.test.jsx
Outdated
Show resolved
Hide resolved
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.
Much better 😃
Feel free to merge once you have @jagabomb 's approval
assets/js/components/ClusterDetails/HanaClusterDetails.stories.jsx
Outdated
Show resolved
Hide resolved
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.
Good job LGTM when @jagabomb approves
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. I would like to also double check this with @abravosuse when he is back. But so far this solution makes sense to me.
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 have reviewed AscsErsClusterDetails With Unregistered Host and HanaClusterDetails With Unregistered Host in the Storybook and they both look good to me.
Description
Check for unregistered hosts to avoid linking and show a warning