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

IN-794 update list of environments to include preprod #3

Merged
merged 2 commits into from Nov 13, 2018

Conversation

Projects
None yet
3 participants
@FacetGraph

FacetGraph commented Oct 22, 2018

Was first thinking of just changing prod to preprod, but not sure if
that 'prod' might be used anywhere. So just added a preprod item to come
before it.

IN-794 update list of environments to include preprod
Was first thinking of just changing prod to preprod, but not sure if
that 'prod' might be used anywhere. So just added a preprod item to come
before it.

@FacetGraph FacetGraph requested review from rabidscorpio and jgoulah Oct 22, 2018

@jgoulah jgoulah added the needs review label Nov 1, 2018

@jgoulah

jgoulah approved these changes Nov 1, 2018

@@ -147,7 +147,7 @@ def run(options, requires_lock = true)
elsif options[:method].match(/force_builda/)
env = "force asset rebuild"
else
env = options[:method][/(dev|qa|production|princess|prod|webs|stage|staging|config)/i, 1] || "other"
env = options[:method][/(dev|qa|production|princess|preprod|prod|webs|stage|staging|config)/i, 1] || "other"
env = "production" if env.match(/prod|webs/)

This comment has been minimized.

@rabidscorpio

rabidscorpio Nov 6, 2018

@FacetGraph This is the problem with deployinator signaling a "Production Deploy Done" instead of "Preprod Deploy" so we need to take that into account.

This comment has been minimized.

@FacetGraph

FacetGraph Nov 6, 2018

grrr yeah looking at that now it looks like you are right. But I tested this and saw PREPROD. Let me dive back into this and confirm whats going on.

This comment has been minimized.

@FacetGraph

FacetGraph Nov 7, 2018

There is quite a few things that could cause problems here. You sort of have to name your environments with knowledge of this code in mind.
It would probably have been better to have a flag for production rather than try to do all this regex matching.
These lines are a little strange, like why would env ever be "force asset rebuild", seems kind of like overloading the env variable here

  if options[:method].match(/config_push/)                              
    env = options[:method].match(/prod/) ? "production" : "princess"    
  elsif options[:method].match(/force_builda/)                          
    env = "force asset rebuild"         if options[:method].match(/config_push/)                              
    env = options[:method].match(/prod/) ? "production" : "princess"    
  elsif options[:method].match(/force_builda/)                          
    env = "force asset rebuild"   

Anyways, I can fix this in the meantime by just adding a ^ to prod on the env.match line

This comment has been minimized.

@rabidscorpio

rabidscorpio Nov 8, 2018

Yeah, there are a lot of etsy specific things in here.

@jgoulah

This comment has been minimized.

jgoulah commented Nov 13, 2018

@FacetGraph what is holding this up? I'd like to get this out

@FacetGraph

This comment has been minimized.

FacetGraph commented Nov 13, 2018

@FacetGraph what is holding this up? I'd like to get this out

Sure I will push this out today

@FacetGraph FacetGraph merged commit 1e828e6 into master Nov 13, 2018

@FacetGraph FacetGraph deleted the in-794 branch Nov 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment