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.

Sign up for GitHub

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
Open

feat(cluster): customisable context path #565

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a1045f
feat(cluster): add support for custom context-path.
Birkbjo Sep 15, 2022
a1cceae
feat(cluster-list): add context-path in cluster-list
Birkbjo Sep 15, 2022
cf461d7
test: fix tests
Birkbjo Sep 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 14 additions & 10 deletions packages/cluster/src/commands/list.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const formatStatus = status => {

const run = async function (argv) {
const clusters = await listClusters(argv)

const anyCustomContext = clusters.some(
cluster => cluster.contextPath !== ''
)
Comment on lines +39 to +41
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

const table = new Table({
head: [
'Name',
Expand All @@ -45,20 +47,22 @@ const run = async function (argv) {
'DHIS2 Version',
'DB Version',
'Status',
],
].concat(anyCustomContext ? 'Context Path' : []),
})

await Promise.all(
clusters.map(async cluster => {
const status = await getStatus(cluster)
table.push([
chalk.blue(cluster.name),
cluster.port,
cluster.channel,
cluster.dhis2Version,
cluster.dbVersion,
formatStatus(status),
])
table.push(
[
chalk.blue(cluster.name),
cluster.port,
cluster.channel,
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 •
edited
Loading

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

)
})
)

Expand Down
4 changes: 2 additions & 2 deletions packages/cluster/src/commands/up.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,8 +138,8 @@ module.exports = {
},
customContext: {
alias: 'c',
desc: 'Serve on a custom context path',
type: 'boolean',
desc: 'Serve on a custom context path. If used as a flag, the name of the cluster will be used.',
type: 'string',
},
variant: {
desc: 'Append variant options to the image',
Expand Down
10 changes: 9 additions & 1 deletion packages/cluster/src/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ async function resolveConfiguration(argv = {}) {
// resolve specials...
resolved.dhis2Version = resolved.dhis2Version || resolved.name
resolved.dbVersion = resolved.dbVersion || resolved.dhis2Version
resolved.contextPath = resolved.customContext ? `/${resolved.name}` : ''
resolved.contextPath = resolveCustomContextPath(resolved)

resolved.dockerImage = makeDockerImage(
resolved.image,
Expand Down Expand Up @@ -163,6 +163,14 @@ module.exports.makeEnvironment = cfg => {
return env
}

const resolveCustomContextPath = resolved => {
let contextPath = resolved.customContext
if (contextPath === '' || contextPath === true) {
contextPath = resolved.name
}
return contextPath ? `/${contextPath}` : ''
}

// This has to match the normalization done by docker-compose to reliably get container statuses
// from https://github.com/docker/compose/blob/c8279bc4db56f49cf2e2b80c8734ced1c418b856/compose/cli/command.py#L154
const normalizeName = name => name.replace(/[^-_a-z0-9]/g, '')
Expand Down