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

Add functionality to copy revision on activation. #43

Merged
merged 1 commit into from
Dec 10, 2015

Conversation

arenoir
Copy link
Contributor

@arenoir arenoir commented Nov 12, 2015

This pr adds a config option copyToKey. When defined the activate function copies the value of the specified version to the copyToKey in redis. This makes it possible to serve the index page from nginx.

// deploy.js (sync)
module.exports = function(environment){
  var ENV = {
  };

  if (environment === 'production') {
    Env.redis = {
      copyToKey: 'my-app:active-version'
    }
  };
  return ENV;
};

Here is the corresponding nginx config.

#this sets a variable $app_version from a url parameter version or defaults to 'current-copy'
map $arg_version $app_version {
    default "active-version";
    ~^\w+ $arg_version;
}
upstream redisbackend{
    server 127.0.0.1:6379;
}
server {
    location / {
        set $redis_key my-app:$app_version;
        redis_pass redisbackend;
        default_type text/html;
    }
}

@ghedamat
Copy link
Contributor

@arenoir thanks for this! It's an interesting use!

one thought,
maybe we should put your sample config in the README as well so folks can have an idea on the use case for this, not sure if in the config option section or in a separate section..

I also wonder if this could be done entirely in nginx (more curiosity than anything) but as @lukemelia pointed out out-of-band this is pretty good for simple use cases.

@arenoir
Copy link
Contributor Author

arenoir commented Nov 12, 2015

@ghedamat, I will add the use case to the README. The NGINX redis module is very limited. I don't think it is possible to do this all in NGINX because multiple lookups are not possible.

I am also going to add the keyPrefix to the copyToKey. Perhaps copyToKey should be named copyToSuffix?

@lukemelia
Copy link
Contributor

How about activeContentSuffix?

@arenoir
Copy link
Contributor Author

arenoir commented Nov 12, 2015

@lukemelia activeContentSuffix sounds good to me.

Should I add a boolean that explicitly enables this feature or is the presence of activeContentSuffix enough?

@arenoir
Copy link
Contributor Author

arenoir commented Nov 12, 2015

I changed the the key to activeContentSuffix updated the README and added a copyOnActivate config boolean.

The more I think about it the more I feel copyOnActivate should be default behavior. I mean whats the cost of one redis entry?

@lukemelia
Copy link
Contributor

@arenoir I don't think we need an extra boolean flag. I think we can base it on the presence of activeContentSuffix. I am amenable to giving this a default value. Not sure what... maybe "active-content"? WDYT @achambers

Also need to consider how/whether this works with people using redis plugin without revision-data plugin.

@ghedamat
Copy link
Contributor

@arenoir @lukemelia I'm ok with a default as well, and +1 for killing the boolean flag

You probably considered this already but, If I read the current version of the implementation correctly, if copyOnActivate is set we won't do the other part of the "normal" activation process, as mentioned above, I think we should do both or only the "normal" one.

@ghedamat
Copy link
Contributor

ghedamat commented Dec 3, 2015

@arenoir @lukemelia ping?

@arenoir arenoir force-pushed the copy-on-activate branch 2 times, most recently from 7d4f946 to 3b47c95 Compare December 3, 2015 20:41
@arenoir
Copy link
Contributor Author

arenoir commented Dec 3, 2015

@ghedamat, I removed the copyOnActivate boolean and rebased with master. So if activeContentSuffix is present it copies the content and also copies the active version key to the activationSuffix.

@ghedamat
Copy link
Contributor

ghedamat commented Dec 9, 2015

@arenoir sorry, could you do another rebase and then we take it for the final review/merge?

…to do the lookup as redis support is very limited.

change copyToKey to activeContentSuffix add to readme

update readme

remove copyOnActivate boolean

update tests
@arenoir
Copy link
Contributor Author

arenoir commented Dec 10, 2015

@ghedamat, okay np I rebased with master... take a look.

@lukemelia
Copy link
Contributor

Nice work @arenoir. Thanks!

lukemelia added a commit that referenced this pull request Dec 10, 2015
Add functionality to copy revision on activation.
@lukemelia lukemelia merged commit 0312eac into ember-cli-deploy:master Dec 10, 2015
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.

None yet

3 participants