-
Notifications
You must be signed in to change notification settings - Fork 78
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
JSUI-2786 Added beta and feature deployments to CDN #1550
Conversation
.travis.yml
Outdated
on: | ||
repo: coveo/search-ui | ||
all_branches: true | ||
condition: "$IS_FEATURE == true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I might not have enough permissions to revert a bad build, bad release or bad upload to CDN if I've made a mistake. I'm going to wait until the rest of this file is approved and @olamothe is available before making the following change:
branches:
only:
- master
- "/^.*release.*$/"
- "/2\\.[0-9]+\\.[0-9]+/"
is going to be changed to
branches:
only:
- master
- "/^.*release.*$/"
- "/2\\.[0-9]+\\.[0-9]+/"
- "/^JSUI-[0-9]+$/"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't IS_FEATURE
be true
only for the last branch format? ^JSUI-[0-9]+$
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and every other should be false
for said format. So it should work, but in the event that I missed something, I wanted to get this reviewed first.
Adding this line will run every above conditions on ^JSUI-[0-9]+$
branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove the $ at the end, branches name with something else after the JIRA tag are valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I'll count those as feature branches too.
.travis.yml
Outdated
- export SOURCE_BRANCH=${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH} | ||
- export IS_FORKED_PULL_REQUEST=$([[ $TRAVIS_PULL_REQUEST != false && $TRAVIS_PULL_REQUEST_SLUG != $TRAVIS_REPO_SLUG ]] && echo true || echo false) | ||
- export IS_BETA=$([[ $TRAVIS_TAG =~ .*beta.* && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_RELEASE=$([[ $SOURCE_BRANCH =~ ^.*release.*$|^master$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_FEATURE=$([[ $SOURCE_BRANCH =~ ^JSUI-[0-9]+$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These variables are introduced to improve the readability of the conditions.
They contain "true"
and "false"
instead of 0
and 1
to avoid ambiguity.
IS_RELEASE
is evaluated by the ^.*release.*$|^master$
regular expression instead of the ! $TRAVIS_TAG =~ .*beta.*
regular expression since these conditions are also going to be evaluated on JSUI-*
branches, from now on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice touch screening out fork pull requests 💯
.travis.yml
Outdated
- export SOURCE_BRANCH=${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH} | ||
- export IS_FORKED_PULL_REQUEST=$([[ $TRAVIS_PULL_REQUEST != false && $TRAVIS_PULL_REQUEST_SLUG != $TRAVIS_REPO_SLUG ]] && echo true || echo false) | ||
- export IS_BETA=$([[ $TRAVIS_TAG =~ .*beta.* && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_RELEASE=$([[ $SOURCE_BRANCH =~ ^.*release.*$|^master$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can remove ^master$
from the source branch regex, since our releases occur on release branches? Or is this for the nightly build? If so, I would prefer to create a separate variable IS_NIGHTLY
. I think it will be helpful to distinguish this release from an official release.
For the official release branches, the format, is <year>-<month>-release
. I think we can mark release
as the end of the regex. We could also be more strict if the regex was looking for 4 numbers|dash|some lowercase letters|dash|release
.
e.g. ^[0-9]{4}-[a-z]+-release$
.
But we could keep it as just ends with release
too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I'll remove ^master$
. Additionally, I realized that the requirement for a commit to count as a release isn't the name of the branch, but rather the tag given to the commit. I'll change the condition.
For nightly builds, they correspond with IS_BETA
, since betas are tagged every day.
As for the format, I agree and I'll make it more strict. However, the condition will no longer be on the branch, it'll be on the tag.
.travis.yml
Outdated
on: | ||
repo: coveo/search-ui | ||
all_branches: true | ||
condition: "$IS_FEATURE == true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't IS_FEATURE
be true
only for the last branch format? ^JSUI-[0-9]+$
.travis.yml
Outdated
access_key_id: AKIAYKDJLZIT462WODCI | ||
secret_access_key: | ||
secure: sT8isIiaAIg/Ic7x8cCD3anwCEHAyzG/IWkngmkf5c128NEwUIUdmsQJ1hi4Bw84zk1OOb3id+yXjeaTuKDgIkxe/jk3e2xZ7xElfLNTOln6QLkgsYpKFtGHiKjROOHOb5+Es6L1mxpHWbXH9ebmEhRduUw/BeHWW2MfZmfb434DgvatViRm8DOeROeqlYnbCda5ZELuT5SdwEs/CbExaiSL0WKVoTvcxbe+At4uVnTuc6SYyOvRoblx/q6Ra6mdRipluMs7Go93eu/Oqo7cZ2akrNSi/WBXGqL/wXxe2pYsziSABWjQaqWXD/jKte1ZBx7OXssn88DUKat/RrL7Ak30WHzgY/goiNV+EPT/wtNYWHP3suAzzGqNNMhsPx0lSI7vSUga72J3hV1TOsX9ORosiRm1c2fnmNB9vJdH7yAX7lWpqH2XuOrldVvvTGazSJYx93eBukDViFCZ6egeb+gMT8VQ38uitwd9BZaMhycoujZgSjhx7JnbHUoUdXkdT3XBNV7fINTozWCNJW81iJN+JuLZzneJ/CbjmjkfGcVoFFPCD9rGSj8DbgvWpt8rkwI4ZfHTNOgO9iWgSklpk9r2wgM7Z87V2Zpu8UX/QCFxy8IREtJUo6X9YG9nBoy21wUa9zia2SWpideur9cXb6U0+q84onHVlA0Ehp/1OYs= | ||
bucket: coveo-nprod-binaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should adjust the bucket to coveo-ndev-binaries
here to avoid mixing feature deployments with the official releases.
Same comment for line 111 holding the nightly build.
Small note is these buckets will be accessible under staticdev.cloud
instead of static.cloud
, which I think is good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@olamothe mentioned that, to make it simpler, we should push all of those to the prod CDN. Perhaps we need to have a discussion on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking behind the comment was to keep the prod bucket tidy by just holding official releases, and to keep the traffic separated.
I think it's good to keep our sandbox from sharing resources with our production files. We can delete everything (intentionally/unintentionally) without fear. In the prod bucket, our permissions are restricted, rightly so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I'd expect nightly and feature builds to be in the dev bucket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, dev bucket is perfectly fine.
When I initially opened this JSUI ticket and we had the discussion many months ago, there was no bucket/CDN in dev/qa/prod.
But @samisayegh did the work of terraforming those since then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And for dev, the encrypted secret_access_key
you are using won't work.
We will need to generate a new one for dev account that can only do this operation (push to this particular bucket) in AWS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, I'll do that, then.
- export SOURCE_BRANCH=${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH} | ||
- export IS_FORKED_PULL_REQUEST=$([[ $TRAVIS_PULL_REQUEST != false && $TRAVIS_PULL_REQUEST_SLUG != $TRAVIS_REPO_SLUG ]] && echo true || echo false) | ||
- export HAS_BETA_TAG=$([[ $TRAVIS_TAG =~ ^[0-9]+\.[0-9]+\.[0-9]+-beta$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export HAS_RELEASE_TAG=$([[ $TRAVIS_TAG =~ ^[0-9]+\.[0-9]+\.[0-9]+$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_ON_FEATURE_BRANCH=$([[ $SOURCE_BRANCH =~ ^JSUI-[0-9]+ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Betas are triggered by a tagged commit on the master
branch and releases are triggered by a tagged commit on a release branch. Should I also enforce the branch or only enforce the tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with just one or the other. I think tag is enough.
Unfortunately, the changes I just made changed the behaviour of the CI a bit too much, so I must request a new review 😅 |
- export SOURCE_BRANCH=${TRAVIS_PULL_REQUEST_BRANCH:-$TRAVIS_BRANCH} | ||
- export IS_FORKED_PULL_REQUEST=$([[ $TRAVIS_PULL_REQUEST != false && $TRAVIS_PULL_REQUEST_SLUG != $TRAVIS_REPO_SLUG ]] && echo true || echo false) | ||
- export HAS_BETA_TAG=$([[ $TRAVIS_TAG =~ ^[0-9]+\.[0-9]+\.[0-9]+-beta$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export HAS_RELEASE_TAG=$([[ $TRAVIS_TAG =~ ^[0-9]+\.[0-9]+\.[0-9]+$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_ON_FEATURE_BRANCH=$([[ $SOURCE_BRANCH =~ ^JSUI-[0-9]+ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd go with just one or the other. I think tag is enough.
.travis.yml
Outdated
access_key_id: AKIAYKDJLZIT462WODCI | ||
secret_access_key: | ||
secure: sT8isIiaAIg/Ic7x8cCD3anwCEHAyzG/IWkngmkf5c128NEwUIUdmsQJ1hi4Bw84zk1OOb3id+yXjeaTuKDgIkxe/jk3e2xZ7xElfLNTOln6QLkgsYpKFtGHiKjROOHOb5+Es6L1mxpHWbXH9ebmEhRduUw/BeHWW2MfZmfb434DgvatViRm8DOeROeqlYnbCda5ZELuT5SdwEs/CbExaiSL0WKVoTvcxbe+At4uVnTuc6SYyOvRoblx/q6Ra6mdRipluMs7Go93eu/Oqo7cZ2akrNSi/WBXGqL/wXxe2pYsziSABWjQaqWXD/jKte1ZBx7OXssn88DUKat/RrL7Ak30WHzgY/goiNV+EPT/wtNYWHP3suAzzGqNNMhsPx0lSI7vSUga72J3hV1TOsX9ORosiRm1c2fnmNB9vJdH7yAX7lWpqH2XuOrldVvvTGazSJYx93eBukDViFCZ6egeb+gMT8VQ38uitwd9BZaMhycoujZgSjhx7JnbHUoUdXkdT3XBNV7fINTozWCNJW81iJN+JuLZzneJ/CbjmjkfGcVoFFPCD9rGSj8DbgvWpt8rkwI4ZfHTNOgO9iWgSklpk9r2wgM7Z87V2Zpu8UX/QCFxy8IREtJUo6X9YG9nBoy21wUa9zia2SWpideur9cXb6U0+q84onHVlA0Ehp/1OYs= | ||
bucket: coveo-nprod-binaries |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking behind the comment was to keep the prod bucket tidy by just holding official releases, and to keep the traffic separated.
I think it's good to keep our sandbox from sharing resources with our production files. We can delete everything (intentionally/unintentionally) without fear. In the prod bucket, our permissions are restricted, rightly so.
- export HAS_RELEASE_TAG=$([[ $TRAVIS_TAG =~ ^[0-9]+\.[0-9]+\.[0-9]+$ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_ON_FEATURE_BRANCH=$([[ $SOURCE_BRANCH =~ ^JSUI-[0-9]+ && $IS_FORKED_PULL_REQUEST = false ]] && echo true || echo false) | ||
- export IS_NIGHTLY=$([[ $HAS_BETA_TAG = true && $TRAVIS_EVENT_TYPE = cron ]] && echo true || echo false) | ||
- export IS_PULL_REQUEST_PUSH_BUILD=$([[ $TRAVIS_PULL_REQUEST = false && $IS_ON_FEATURE_BRANCH = true ]] && echo true || echo false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only true
on continuous-integration/travis-ci/push
builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking back at this, I'm realizing that this would likely cause push builds to be triggered on JSUI-*
branches, whether they have a PR or not. However, for certain features like QuestionAnswering (JSUI-2049) that don't necessarily have an open PR, this could be beneficial.
@samisayegh @ThibodeauJF any opinion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pushing all JSUI-*
branches is fine. Let's start with this. If we see it is too for some reason, we can adjust.
- echo $TRAVIS_BRANCH | ||
- echo $TRAVIS_PULL_REQUEST_BRANCH | ||
- echo $TRAVIS_PULL_REQUEST | ||
- echo $SOURCE_BRANCH | ||
- echo $IS_FORKED_PULL_REQUEST | ||
- echo $HAS_BETA_TAG | ||
- echo $HAS_RELEASE_TAG | ||
- echo $IS_ON_FEATURE_BRANCH | ||
- echo $IS_NIGHTLY | ||
- echo $IS_PULL_REQUEST_PUSH_BUILD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I keep those echo
? In case anything happens, those would most likely help find the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, should we put the name of the tag next to the boolean value? I imagine as it is now we would only see a series of true
and false
in the console?
- if [ "x$TRAVIS_TAG" != "x" ]; then yarn run minimize ; fi | ||
- yarn run unitTests | ||
- if [ "x$TRAVIS_TAG" != "x" ]; then yarn run accessibilityTests ; fi | ||
- if [ $IS_PULL_REQUEST_PUSH_BUILD = false ]; then yarn run unitTests ; fi | ||
- if [[ "x$TRAVIS_TAG" != "x" && $IS_PULL_REQUEST_PUSH_BUILD = false ]]; then yarn run accessibilityTests ; fi | ||
- set +e | ||
- yarn run uploadCoverage | ||
- if [ $IS_PULL_REQUEST_PUSH_BUILD = false ]; then yarn run uploadCoverage ; fi | ||
- set -e | ||
- yarn run validateTypeDefinitions | ||
- if [ $IS_PULL_REQUEST_PUSH_BUILD = false ]; then yarn run validateTypeDefinitions ; fi | ||
after_success: | ||
- if [ "x$TRAVIS_TAG" != "x" ]; then bash ./deploy.doc.sh ; fi | ||
- if [[ "x$TRAVIS_TAG" != "x" && $IS_PULL_REQUEST_PUSH_BUILD = false ]]; then bash ./deploy.doc.sh ; fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On continuous-integration/travis-ci/push
builds, we probably don't want to:
- minimize
- run unit tests
- upload coverage
- validate type definitions
- deploy documentation
on: | ||
repo: coveo/search-ui | ||
all_branches: true | ||
condition: "$IS_PULL_REQUEST_PUSH_BUILD == true" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deployment providers can't be run on a continuous-integration/travis-ci/pr
build. There's probably a way to make that work, but continuous-integration/travis-ci/pr
pull changes from master
and their builds seem to be mostly for testing anyways.
@@ -106,3 +151,4 @@ branches: | |||
- master | |||
- "/^.*release.*$/" | |||
- "/2\\.[0-9]+\\.[0-9]+/" | |||
- "/^JSUI-[0-9]+/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continuous-integration/travis-ci/push
builds are triggered by a Travis option on the repo that happened to be already selected, but they weren't triggered before due to the lack of this line.
- echo $TRAVIS_BRANCH | ||
- echo $TRAVIS_PULL_REQUEST_BRANCH | ||
- echo $TRAVIS_PULL_REQUEST | ||
- echo $SOURCE_BRANCH | ||
- echo $IS_FORKED_PULL_REQUEST | ||
- echo $HAS_BETA_TAG | ||
- echo $HAS_RELEASE_TAG | ||
- echo $IS_ON_FEATURE_BRANCH | ||
- echo $IS_NIGHTLY | ||
- echo $IS_PULL_REQUEST_PUSH_BUILD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, should we put the name of the tag next to the boolean value? I imagine as it is now we would only see a series of true
and false
in the console?
@samisayegh That's what I originally tried to do, but Travis outputs every command above their respective output and even collapses the output of |
https://coveord.atlassian.net/browse/JSUI-2786
This PR introduces the two following deployments to CDN:
-beta
tagcontinuous-integration/travis-ci/push
buildsAs well as introducing those two deployments, this PR adds the
continuous-integration/travis-ci/push
build, which is responsible for deploying to the CDN.