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

Environment Indicator #86

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

Environment Indicator #86

wants to merge 2 commits into from

Conversation

enotick
Copy link

@enotick enotick commented May 20, 2020

@RobLoach can you please take a look at environment indicator settings. Let's test it like this if this is ok and then we can refactor if you'd like. Thanks.

* origin/master:
  Don't send email on non-live environments. (#80)
  Add warning that settings.php will be overwritten. (#85)
  Add install file to the profile for config changes. (#78)
  Add devel and stage_file_proxy as dev requirements.
  Update readme after new contrib and install profile.
  Add install profile for installing common modules.
  Add contrib modules most commonly used.
  Update versions of required packages in composer.json.
@enotick enotick requested a review from RobLoach May 20, 2020 15:43
@RobLoach RobLoach changed the title Env indicator Environmental Indicator May 22, 2020
@RobLoach RobLoach changed the title Environmental Indicator Environment Indicator May 22, 2020
@mikemccaffrey
Copy link

I'm fine with installing the indicator, but I would like to clean this code up a bit:

  • Let remove everything that is commented out.
  • We have virtually no sites on acquia, so we probably don't need to check those variables.

However, there is also a bit of a fundamental issue: The colors have been chosen to discourage developers from making changes on live instead of dev, but for site editors we actually want the opposite. They should make content changes on live and instead be warned if they are making changes to dev or test.

Is there a way we can tie the indicator colors to roles or permissions so we don't alarm the site editors with a bright red admin bar on the live site?

@enotick
Copy link
Author

enotick commented May 22, 2020

@mikemccaffrey no the indicator is for developers, for editors we can hide it completely. I am sure you could do something like an event subscriber or preprocess and get the role there but this is generally speaking not the original purpose of the module - it was developed to work with configuration and alert people to not do config save on live.
In terms of removing Acquia I think we should keep it - we may have sites on Acquia later on but more precisely this snippet has benefited from being low maintenance in terms of environment detection over years. It's taken from BLT that works with Acquia and Pantheon and the splits specifically as well as detection changes quite a bit - being able to copy paste saved us a lot of time in the past and allowed to rely on BLT community and maintainers and not re-invent the wheel. While we can clean it up - it's just easier to maintain as is - speaking from experience.

@mikemccaffrey
Copy link

It would just be weird to have this one single piece of code reference acquia in the entire project. If were were to support acquia with this project, things would be much more complicated.

@enotick
Copy link
Author

enotick commented May 25, 2020

Hmmmm I can see that... idk I think having two hostings support could be nice and I also agree that this increases CI complexity for sure. Maybe something generator could solve for us. Ok let's in the interim remove Acquia out of the picture but maybe be have it for the future stage as a goal.

Copy link

@RobLoach RobLoach left a comment

Choose a reason for hiding this comment

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

Looks pretty great! @mikemccaffrey Mind trying out the Kalacolors?

@RobLoach RobLoach removed their assignment Sep 10, 2021
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