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

Dx improvements #1758

Merged
merged 3 commits into from
Jan 6, 2025
Merged

Dx improvements #1758

merged 3 commits into from
Jan 6, 2025

Conversation

xendk
Copy link
Contributor

@xendk xendk commented Nov 14, 2024

Minor improvements to docker setup.

Copy link
Contributor

@kasperg kasperg left a comment

Choose a reason for hiding this comment

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

👍

I do not agree with removing the default COMPOSE_PROJECT_NAME. I have tried to argue my case. If you still insist then let's merge.

.env.example Outdated
@@ -1,4 +1,3 @@
COMPOSE_PROJECT_NAME=dpl-cms
Copy link
Contributor

@kasperg kasperg Nov 14, 2024

Choose a reason for hiding this comment

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

Personally I do not see this as a DX improvement.

We have a handful of different aspects of the project which require the host name to be dpl-cms.docker including OpenID Clients for Adgangsplatformen set up by DBC.

I know that COMPOSE_PROJECT_NAME will default to the name of the current directory, that Git will use the repository name by default when cloning, that GitHub Actions checkout will not provide a custom directory name by default etc.

If some of these assumptions do not hold then you are going to run into problems with non-obvious solutions.

To me it is preferable to avoid these by ensuring a default host name.

If someone wants to run multiple instances they are free to edit their local .env file. This would be a one-time workflow for them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have a handful of different aspects of the project which require the host name to be dpl-cms.docker

I'll argue that the two TS files should be fixed so they can sniff out the proper hostname. This is precisely the kind of hardcoding that can come back to bite you some day.

As for the MD files: One could argue for replacing dpl-cms.docker with <your local environment>, but that kind of placeholdering quickly grows tiresome. I'm sure that anyone that's actually using a different hostname can mentally substitute.

including OpenID Clients for Adgangsplatformen set up by DBC.

That's unfortunate, and I guess they don't support wildcards either? But I'd more consider that a limitation of running more than one instance. In my case, it means I can't use Adgangsplatformen at all, because I don't have any .docker mapping (maybe I can get past that with good old /etc/hosts, but the DX will suck).

Docker compose defaults to using the directory name and hardcoding it
makes running two instances troublesome.
The TLD used depends on the developers setup, some use `docker`,
others `local`, so make it configurable.
@xendk xendk merged commit 78d299d into develop Jan 6, 2025
19 checks passed
@xendk xendk deleted the dx-improvements branch January 6, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants