-
Notifications
You must be signed in to change notification settings - Fork 7
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(cluster): customisable context path #565
base: master
Are you sure you want to change the base?
Conversation
3ae135a
to
8776455
Compare
d3d2f29
to
17c6ae1
Compare
17c6ae1
to
cf461d7
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.
Haven't tested myself but this looks good, thanks @Birkbjo! Some minor comments, not blocking.
const anyCustomContext = clusters.some( | ||
cluster => cluster.contextPath !== '' | ||
) |
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 we just always show the context, even if none are custom?
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 could. The idea was that this is most likely not relevant to most users. I think very few are actually using this context-path, and it might be confusing if you don't know what it does?
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 could go either way here... I also think it would be confusing if d2 cluster list
sometimes renders in a different format if you happen to have a custom context on one cluster you're running. It would make any kind of parsing of the output difficult as well, though we're not exactly well setup for that as it is
cluster.dhis2Version, | ||
cluster.dbVersion, | ||
formatStatus(status), | ||
].concat(anyCustomContext ? cluster.contextPath : []) |
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.
Maybe cluster.contextPath || '/'
?
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.
If you think we should show this regardless of any set custom-context, I agree, we should probably do that.
Or do you mean we should show '/' instead of an empty cell for non-custom contexts?
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 mean we should show /
instead of an empty cell for non-custom contexts, since that's the path that they can use to access the instance
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'll defer to Austin's review on this. I'm not super familiar with this bit of functionality as a user, changes look good to me 👍️
Previously the
--customContext
option for theup
-command was just a boolean, and it used thecluster name
for thecontextPath
when this was set. I think it should be possible to set this independently of thecluster name
, as it's very useful if you would want additional levels, eg./dhis2/dev
. This is somewhat related to CLI-75, since having a cluster with a path-delimeter (/), results in very buggy behavior.I've tried to implement this in a non-breaking way. Eg. we still support using a flag
-c
, and we will then use thecluster name
.