Skip to content
This repository has been archived by the owner on Mar 19, 2024. It is now read-only.

💊 Fix Azure repo cloning #51

Merged
merged 1 commit into from
Sep 26, 2016
Merged

Conversation

st3v3nhunt
Copy link
Contributor

@st3v3nhunt st3v3nhunt commented Sep 26, 2016

Azure deployments are breaking because it doesn't have access to the GitLab repo. There are 2 steps involved in fixing it:

  • Add the deployment key to the GitLab repo
  • Update the submodule so it is requested over SSH rather than https (which means the deployment key will work, as opposed to having to send username:password)

I originally thought (see below) the vars in the env would be overridden by the .env file vars but this is not the case

We will never modify any environment variables that have already been set

https://www.npmjs.com/package/dotenv#what-happens-to-environment-variables-that-were-already-set

However, this has got me thinking about the whether handling the env vars via submodules is a good idea. This is the second thing that has had to be dealt with differently due to the approach (the other being the ignoring of submodules in Travis).

Some of the original reasons for taking this approach was to:

  • have an automated setup mechanism for development environments
  • have a central place where working vars are stored
  • other things I've forgotten about

If the submodule was removed the repo can still live and contain development versions of the vars for reference so no loss there. Having an automated setup for development environments - well that could just be a case of accepting that the environment variables need to available. Which does take a little bit of remembering but can be included in the README.md. A benefit of doing that would be that the dotenv package could be removed and all environments would be using the same mechanism.

Thoughts?

This does mean the Azure deployments are now going to be using the env vars from the repo which is the same as the development environment but no other environment i.e. CI, Heroku. This is not ideal.

Whether as part of this change or another one, the way of handling env vars need to be brought back to be consistent. Looking at the docs there are other ways to load env vars e.g. start the app with the file which would be something we could do just for development and let all other environments use the env vars they have.

Some thought required. I think having another look at https://12factor.net/config might be a good idea.

@st3v3nhunt st3v3nhunt temporarily deployed to connecting-to-services-pr-51 September 26, 2016 10:52 Inactive
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.173% when pulling 1a4880c on feature/ssh-submodule into c42e802 on master.

@neilbmclaughlin neilbmclaughlin temporarily deployed to connecting-to-services-pr-51 September 26, 2016 12:44 Inactive
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.173% when pulling b901c4d on feature/ssh-submodule into 7624276 on master.

@neilbmclaughlin
Copy link
Contributor

I'd be happy to see dotenv removed and allow require-environment-variables to trap missing environment variables.

In the longer term it would be good to have a dotfile (perhaps as part of the project) for developers which sets up an environment for a new developer (install node, clone repo, setup public env vars) and a private repo for secret env vars such as the the api key.

In would also be useful to have scripts for the creation of a heroku and azure apps (including env vars) which will stand as documentation of our setup rather than requiring manual inspection through the UI.

@neilbmclaughlin
Copy link
Contributor

Do you want me to merge this, remove the usage of dotenv or wait for you to respond?

@st3v3nhunt
Copy link
Contributor Author

I'll sort it out by removing dotenv and adding some notes to the README

@st3v3nhunt st3v3nhunt temporarily deployed to connecting-to-services-pr-51 September 26, 2016 15:06 Inactive
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 93.117% when pulling aa2f333 on feature/ssh-submodule into 7624276 on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.06%) to 93.117% when pulling 3ad6f8c on feature/ssh-submodule into 7624276 on master.

@neilbmclaughlin neilbmclaughlin merged commit 7f2fc29 into master Sep 26, 2016
@neilbmclaughlin neilbmclaughlin deleted the feature/ssh-submodule branch September 26, 2016 15:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants