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

Cleanup and bump glide.yaml and fix unit tests #129

Closed
wants to merge 2 commits into from

Conversation

zhouzhuojie
Copy link
Contributor

@zhouzhuojie zhouzhuojie commented Jul 6, 2017

This pr is not urgent at all. Just want to send it anyway incase there's a need to bump glide in the future.

Spent some time clean up glide.yaml and finally got a successful upgrade. Especially the logrus package upgrade, I need to do something like, because glide is case sensitive, and downstream packages require both.

- package: github.com/sirupsen/logrus
- package: github.com/Sirupsen/logrus
  repo: git@github.com:/sirupsen/logrus
  vcs: git

Here's how to run a full clean glide upgrade

glide cc; rm -rf ~/.glide; rm -rf vendor; rm glide.lock; glide --debug up

Also, since some unit tests are broken, it's better to be another pr, but it relies on the glide update, so I've put them here. Really hope that Github can support stack code review.

server/plugins/codeflow/api_auth_test.go
server/plugins/git_sync/git_sync_test.go
server/plugins/kubedeploy/deployments_test.go
server/plugins/kubedeploy/loadbalancers_test.go
server/plugins/route53/route53_test.go
server/plugins/webhooks/github/github_webhooks_test.go

- package: github.com/maxwellhealth/bongo
- package: github.com/pborman/uuid
- package: github.com/spf13/cobra
- package: github.com/spf13/viper
- package: github.com/spf13/pflag
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one is needed to get the most recent pflag update, otherwise it breaks cobra.

- package: github.com/sirupsen/logrus
- package: github.com/Sirupsen/logrus
repo: git@github.com:/sirupsen/logrus
vcs: git
Copy link
Contributor Author

Choose a reason for hiding this comment

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

glide is case sensitive

@zhouzhuojie zhouzhuojie changed the title Cleanup and bump glide.yaml Cleanup and bump glide.yaml and fix unit tests Jul 7, 2017
@zhouzhuojie zhouzhuojie force-pushed the zz/bump-glide branch 16 times, most recently from a405d0e to b88b17c Compare July 10, 2017 18:18
@zhouzhuojie
Copy link
Contributor Author

Also fixed circleci by reusing our docker image to test golang. Now, everything good to go, including tests in all the _test.go, with complete CI support

version: 2
jobs:
  build:
    working_directory: ~/codeflow
    machine:
    ¦ services:
    ¦   - docker
    steps:
    ¦ - checkout
    ¦ - run: cp ./dashboard/.env ./dashboard/.env.development
    ¦ - run: docker build . -t codeflow-test
    ¦ - run: docker run --rm codeflow-test make test

@@ -1,98 +0,0 @@
package kubedeploy_test
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 removed it because the package name should be package kubedeploy, which means it doesn't test the desired package before. Also, the agent is REALLY hard to setup the mock to pass the tests listed here. My 2 cents is that unit tests should be as unit as possible, for example, lots of the logic can be simply tested against the functions defined in kubedeploy/deployment, instead of calling it from agent -- kind of more like an integration test to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is an integration test, while we may not run it out of CI it's required when making changes to kubedeploy. All it needs is a test kube cluster to point at.. Usually we point at dev. I'd like to keep this test, is it getting in the way of the unit 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.

Oh, maybe I need to setup a test cluster to get it working, do we have some notes/docs about how to set it up?

If it's not CI testable, for example, hard to setup kubedeploy in CI, maybe we can put it as code and run it manually as an integration test.

@@ -22,11 +26,13 @@ import:
subpackages:
- jwk
- package: github.com/libgit2/git2go
version: v25
repo: https://gopkg.in/libgit2/git2go.v25.git
vcs: git
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhouzhuojie Why's this change required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not required, I can change it back - probably doesn't matter, before I was just testing and got glide upgrade errors

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, lets keep the orig repository here.

steps:
- checkout
- run: cp ./dashboard/.env ./dashboard/.env.development
- run: make test
- run: docker build . -t codeflow-test
- run: docker run --rm codeflow-test make test
Copy link
Contributor

Choose a reason for hiding this comment

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

This could definitely be sped up considerably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, about 8min :(. Most of the time in the docker build is spent in npm install. I experimented with a lot of options in circleci, (https://circleci.com/gh/checkr/codeflow) really hard to get the libgit2 dependency working correctly, and then the docker build is kind of the last thing I can think to make it fully work. We can have a separate base image (without the npm update/install, pacman install etc.) to speed it up a little bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's ok to build the docker here for now, the make test also does an npm install but I'm guessing it caches better? The other option might be to somehow wait for codeflow to build the docker image and then simply pull it.

@stale-bot
Copy link

stale-bot bot commented Feb 20, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale-bot stale-bot bot added the wontfix label Feb 20, 2018
@stale-bot stale-bot bot closed this Feb 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

3 participants