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

Initial update to circleci v2 config. #2672

Merged
merged 5 commits into from
Aug 29, 2018

Conversation

b4handjr
Copy link
Contributor

@b4handjr b4handjr commented Jul 11, 2018

Should fix #2609

This new config uses the docker-compose configuration to run both the unit tests and a new suite of integration tests. The integration tests can be found here.

The unit tests and integration tests run per commit. These also run on each master/release branch build as well as docker deploys.

I think I have most of the old config added but, of course, this can be changed as needed.

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

Thanks! This is greatly helpful.

See my comments :)

circle.yml Outdated
jobs:
build:
docker:
- image: circleci/node:6
Copy link
Member

Choose a reason for hiding this comment

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

We were using Node v6.9 because that's the latest that was available in the old platform. Now I assume we can use something newer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't specify an exact version in error, but It defaults to 6.14 if I remember correctly. You can view some builds/logs here: https://circleci.com/gh/mozilla/redash/tree/update-circleci

Copy link
Member

Choose a reason for hiding this comment

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

@arikfr There are a lot more modern npm versions on https://hub.docker.com/r/circleci/node/tags/ but I would recommend not doing an update at the same time as this pull request to remove the chance for conflicting issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jezdez I don't think 6.9 is available as a docker image though. Unless you are talking about doing a major update to the code-base, outside of this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Let's update to circleci/node:8, which is the latest LTS and the version I've been working with for along time now, so doubt there will be any issues.

command: |
set -x
docker-compose up -d
sleep 10
Copy link
Member

Choose a reason for hiding this comment

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

This feels brittle... did you try the following:

  1. docker-compose up -d --no-start -> this will create all the necessary containers.
  2. docker-compose start postgres -> this will return when Postgres is running.
  3. Now you can proceed to creating the database and running the tests.

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's pretty slick, I should give that a go since --rm will run the containers anyway.

name: Tag deploy
command: |
set -x
bin/pack "$CIRCLE_TAG"
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to define the tar ball file as an artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this where I need some clarification. Do we just want the artifact as a circleci artifact? Or does it need to be something specific. Circleci 2.0 doesn't have much on the old tarball command, just stating that it was an artifact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at current master builds on circle, there is an artifact with a tar.gz extension. Is this made with the command bin/pack ?

Copy link
Member

Choose a reason for hiding this comment

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

@jrbenny35 Yep, bin/pack basically puts everything in the repo clone into a tarball (except a few things like git history and node_modules etc).

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it just needs to be available as a CircleCI artifact.

@jezdez
Copy link
Member

jezdez commented Jul 12, 2018

@arikfr Just to clarify, are you okay with this PR running the UI tests that are developed over in https://github.com/mozilla/redash-ui-tests here?

@b4handjr
Copy link
Contributor Author

b4handjr commented Aug 2, 2018

Just added some updates. Let me know if there is anything still missing.

@hackebrot
Copy link
Member

Hey @arikfr! 👋

Can you please give this another look and see if we can merge this to master?

Thank you! 🙇

@b4handjr
Copy link
Contributor Author

Any updates on this?

@jezdez
Copy link
Member

jezdez commented Aug 24, 2018

@jrbenny35 I've talked with @arikfr about this and we settled on this:

  • If there is an option to configure Circle CI to optionally run the UI tests (e.g. by clicking a button) when needed, then we should do that.
  • If there isn't an option we agreed that it's too early to use the UI tests in this repo and would like to ask you to remove them for now so we can merge this without it.

@b4handjr
Copy link
Contributor Author

If there is an option to configure Circle CI to optionally run the UI tests (e.g. by clicking a button) when needed, then we should do that.

What is the idea behind this?

@jezdez
Copy link
Member

jezdez commented Aug 24, 2018 via email

@b4handjr
Copy link
Contributor Author

Basically to make sure that the runtime of the tests isn’t increased and minor changes don’t trigger the UI tests by default. Instead when big features land or UI components are changed we could trigger the UI tests on demand from the Circle CI user interface.

What about a branch or a commit string? I don't think there is a button to run tests on circle.

@jezdez
Copy link
Member

jezdez commented Aug 24, 2018

There is also the ability to approve specific jobs of a workflow on Circle CI

@b4handjr
Copy link
Contributor Author

I don't know if this change will work but from my research circleci step halt found here allows the job to be cancelled without returning a failure code.

The string I used is !run-ui-tests. Not sure if it will work until merging though.

@arikfr arikfr merged commit 99dd4dd into getredash:master Aug 29, 2018
@arikfr
Copy link
Member

arikfr commented Aug 29, 2018

Thanks @jrbenny35 for your help wit this! I merged this now and will follow up on any further changes (if needed) myself as it will be easier having access to CircleCI's console.

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.

Migrate to Circle CI v2.0
4 participants