Skip to content
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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Birkbjo
Copy link
Member

@Birkbjo Birkbjo commented Sep 15, 2022

Previously the --customContext option for the up-command was just a boolean, and it used the cluster name for the contextPath when this was set. I think it should be possible to set this independently of the cluster 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 the cluster name.

@Birkbjo Birkbjo force-pushed the feat/cluster-context-path branch 2 times, most recently from 3ae135a to 8776455 Compare September 15, 2022 23:44
@Birkbjo Birkbjo force-pushed the feat/cluster-context-path branch 2 times, most recently from d3d2f29 to 17c6ae1 Compare September 16, 2022 00:05
@Birkbjo Birkbjo changed the title feat(cluster): context path feat(cluster): customisable context path Sep 16, 2022
@Birkbjo Birkbjo unassigned amcgee and ismay Sep 16, 2022
Copy link
Member

@amcgee amcgee left a 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.

Comment on lines +39 to +41
const anyCustomContext = clusters.some(
cluster => cluster.contextPath !== ''
)
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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 : [])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe cluster.contextPath || '/' ?

Copy link
Member Author

@Birkbjo Birkbjo Sep 16, 2022

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?

Copy link
Member

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

Copy link
Member

@ismay ismay left a 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 👍️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants