Skip to content
This repository has been archived by the owner on Aug 23, 2023. It is now read-only.

reorganize scripts + switch to circleCI 2.0 + various new qa scripts and their corresponding fixes #803

Merged
merged 24 commits into from
Jan 2, 2018

Conversation

Dieterbe
Copy link
Contributor

the scripts dir has become a mess and it's time to reorganize.
While we're at it, make scripts more consistent in that they all
start in the root project dir, making it more easy to understand
the file paths used throughout the scripts
  • some other stuff

the scripts dir has become a mess and it's time to reorganize.
While we're at it, make scripts more consistent in that they all
start in the root project dir, making it more easy to understand
the file paths used throughout the scripts
@Dieterbe Dieterbe force-pushed the maintain-scripts branch 4 times, most recently from 6e7a13c to a102f66 Compare December 29, 2017 21:30
@Dieterbe Dieterbe changed the title reorganize scripts reorganize scripts + switch to circleCI 2.0 + various new qa scripts and their corresponding fixes Dec 31, 2017
amazingly, with circleci 1.0 i spent a day trying to get go generate
to work trying to properly install the vendored libraries into their
actual, non-vendored pkg directories, and it never worked, with 3
different approaches.
Now with circleci 2.0 it just seems to work.
(unfortunately, same pager issue still exists on circleCI 2.0)
go get -u golang.org/x/tools/cmd/stringer github.com/tinylib/msgp
go generate $(go list ./... | grep -v /vendor/)
note: deliberately NOT added to CI to enforce anything,
since this stuff is subjective and context dependent
@Dieterbe
Copy link
Contributor Author

Dieterbe commented Dec 31, 2017

after lots of struggling getting ineffassign and go generate to work reliably on circleCI 1.0 (and ultimately failing), I'm pleased to say not only was upgrade to circleci 2.0 pretty smooth, it also made those things work. I'm not quite sure why. (perhaps something was wrong with our go install on the former, or the symlinking of the checkout dir to the path within $GOPATH, or ...) but anyway. now checkout dir is simply the proper dir, which simplifies a lot, and things just work.

If we were to republish this PR commit by commit you'd see this nice pattern of introducing a qa script which results in a build failure, followed by whatever code fixes that make the build succeed again.
however due to the extensive rebasing that visual pattern got lost, but I think the commits speak for themselves :)

Copy link
Member

@woodsaj woodsaj left a comment

Choose a reason for hiding this comment

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

The order of the build steps needs to be adjusted so that steps are run in order of complexity. Simple tests should run first, and complex tests last. This ensures that the more resource intensive steps only run if all other steps have passed.

- run: scripts/qa/gofmt.sh
- run: scripts/qa/ineffassign.sh
- run: scripts/qa/go-generate.sh
- run: scripts/qa/end2end.sh
Copy link
Member

Choose a reason for hiding this comment

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

end2end takes a long time, we should only run it after all other qa tests have passed.

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've tried to take runtime into account. however some things are more important than others. let's say there's a code change that breaks end2end completely, then it's pointless to first obsess over typo's and minor code linting issues, and only find out after a few iterations of code pushing that the code is wrong and needs to be rewritten completely. I do think most of these code health qa scripts can be run in parallel which is something i want to migrate towards (at some point)

Copy link
Member

Choose a reason for hiding this comment

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

I think that is backwards. I think the linting issues and typo's should be resolved first.

By your logic, you will have to wait 20minutes between each code iteration before being told you have a simple typo. It seems insane to be running a full end2end test mulitple times as you fix minor issues that could be reported in a few seconds. With your approach it will take 10x longer to get the same amount of work done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. will fix

artifacts:
- build

deployment:
Copy link
Member

Choose a reason for hiding this comment

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

deployment does not appear to be a supported top level key. Looks like you need to use workflows

@woodsaj
Copy link
Member

woodsaj commented Jan 2, 2018

Your .circleci/config.yml doesnt look to be following the 2.0 spec. https://circleci.com/docs/2.0/configuration-reference/

I think you need to use workflows and break this up into separate jobs. See https://github.com/grafana/worldping-gw/blob/master/.circleci/config.yml as an example.

I would use a Docker image to build and test, then pass the built assets through (persist_to_workspace, attach_workspace) to a second job that uses a VM (machine:true) to build the packages, docker image and run the end2end test. If the branch is master it should also deploy.

@Dieterbe Dieterbe force-pushed the maintain-scripts branch 2 times, most recently from c067130 to ad46a4b Compare January 2, 2018 09:05
@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jan 2, 2018

at this point the goal is to upgrade to circle 2.0 and do all the script rework stuff.
you're right that the deploy stanza was out of date and i have now set it up as described on
https://circleci.com/docs/2.0/configuration-reference/#deploy
the workflow stuff brings a lot of nice features (parrallelism, scheduled runs instead of on-push, etc) but that can wait.

this way we can upload all build artifacts without all the tmp stuff
also separate dir for packages (because it seems a bit cleaner)
@Dieterbe
Copy link
Contributor Author

Dieterbe commented Jan 2, 2018

how does this look now? good to merge? the change in artifacts meant going from 1:25 upload time to 15s

@woodsaj
Copy link
Member

woodsaj commented Jan 2, 2018

LGTM

@Dieterbe Dieterbe merged commit 991d61e into master Jan 2, 2018
@Dieterbe Dieterbe deleted the maintain-scripts branch January 2, 2018 16:00
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.

2 participants