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

Document our contributed modules #1073

Merged
merged 3 commits into from
Apr 18, 2024
Merged

Conversation

loganmcdonald-noaa
Copy link
Member

What does this PR do? 🛠️

#1028

What does the reviewer need to know? 🤔

Not sure how useful the Github action actually is, and it won't catch dev-only modules. Those aren't the most important, but still nice to have information on.

@@ -22,7 +22,6 @@
"drupal/core-composer-scaffold": "^10.2",
"drupal/core-project-message": "^10.2",
"drupal/core-recommended": "^10.2",
"drupal/devel": "^5.1.2",
Copy link
Member Author

Choose a reason for hiding this comment

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

I've always been confused why we have both here and in dev-only 🤔 . Now that I'm thinking about it, it might not belong in dev, it might belong here but excluded via config_split?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm genuinely not sure how this stuff got into our composer file in the first place. I think it was from a scaffold and we just never changed it. That said, what you've said here makes sense. The only reason I can think of for only having drupal/devel in the dev-only section is so it's not even installed in prod, and thus cannot possibly be exploited. But... I dunno how much of a concern that is?

@loganmcdonald-noaa loganmcdonald-noaa marked this pull request as ready for review April 17, 2024 19:33
Copy link
Collaborator

@greg-does-weather greg-does-weather left a comment

Choose a reason for hiding this comment

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

right on

@@ -22,7 +22,6 @@
"drupal/core-composer-scaffold": "^10.2",
"drupal/core-project-message": "^10.2",
"drupal/core-recommended": "^10.2",
"drupal/devel": "^5.1.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm genuinely not sure how this stuff got into our composer file in the first place. I think it was from a scaffold and we just never changed it. That said, what you've said here makes sense. The only reason I can think of for only having drupal/devel in the dev-only section is so it's not even installed in prod, and thus cannot possibly be exploited. But... I dunno how much of a concern that is?


_Added in January 2024_

We were looking for a way to disable certain modules locally (namely, samlauth and autologout) which only pertain to our cloud environments. We also wanted to set different configurations for autologout timing in beta than other cloud environments, fine tuning our settings for our production environment. This also allows us to restrict certain modules to dev-use only like `devel`. We could also use the `--no-dev` flag, but since the composer install in cloud.gov is not something we directly control, this flag is not used. `config_split` is the primary module for splitting configuration across environments. It required a complete reorganization of our configuration files.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh. So cloud.gov installs it anyway. Well then, seems like config-split makes the most sense then. 😂

@loganmcdonald-noaa loganmcdonald-noaa added this pull request to the merge queue Apr 18, 2024
Merged via the queue into main with commit 5d3ec6f Apr 18, 2024
12 checks passed
@loganmcdonald-noaa loganmcdonald-noaa deleted the lmm/1028-modules-docs branch April 18, 2024 16:51
This pull request was closed.
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.

2 participants