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

Ddfher 186 bnf dev setup #1970

Merged
merged 3 commits into from
Jan 20, 2025
Merged

Ddfher 186 bnf dev setup #1970

merged 3 commits into from
Jan 20, 2025

Conversation

xendk
Copy link
Contributor

@xendk xendk commented Jan 14, 2025

Link to issue

https://reload.atlassian.net/browse/DDFHER-186

Description

Add setup for running both a CMS and a BNF site in one environment.

Copy link
Collaborator

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

I both like and dislike the approach taken here :)
At one point I think it solves the problem of managing the two sites in develop nicely.

At another point we keep on bragging about the genius idea about using the same Drupal instance for dpl-cms and bnf and the automatic benefits - which I agree upon.
But here we have two docker compose files that are almost identical and we have two settings.php files. And as far as I see it, there is quite a big risk that we forget something maintaining it in the future

dev:bnfcli:
summary: dev:cli for BNF site, a noop if BNF not enabled
status:
- "[[ -z \"{{ .DOCKER_COMPOSE_FILES_BNF}}\" ]]"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be nice with and error to the end user if BNF is not enabled when using this cmd.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And I can see now it is because you use it internally in the Taskfile. The thing is that we use "dev:cli" directly quite a lot in our team. So, as it is with these changes, if we call task dev:bnfcli nothing would happen and the developer would not know why, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, task prints "bnfcli is up-to-date". Not ideal, but if you're in a position where you actually use the task, you'll we aware of this quirk.

assets/sites.php Outdated
Comment on lines 10 to 17
$host = $_SERVER['HTTP_HOST'];
if (preg_match('/^bnf-/', $host)) {
$sites[$host] = 'bnf';
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this prefix rule should be documented somewhere so people know it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes, but where?

@xendk xendk force-pushed the ddfher-186-bnf-dev-setup branch from 57cb52b to f9bf216 Compare January 16, 2025 14:38
@spaceo spaceo self-requested a review January 20, 2025 11:46
Copy link
Collaborator

@spaceo spaceo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Note: We talked about extending docker.compose.yml in the bnf composer file. Could be worth checking out.

xendk added 3 commits January 20, 2025 13:41
BNF will need to optionally spin two sites up in the local docker
setup, and this makes it simpler.

Ref DDFHER-186
Ref DDFHER-186
@xendk xendk force-pushed the ddfher-186-bnf-dev-setup branch from f9bf216 to 2f5a1cc Compare January 20, 2025 12:44
@xendk xendk merged commit 984daf1 into develop Jan 20, 2025
20 checks passed
@xendk xendk deleted the ddfher-186-bnf-dev-setup branch January 20, 2025 13:04
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