-
Notifications
You must be signed in to change notification settings - Fork 31
Conversation
4fc76bf
to
ae0d9d1
Compare
To get the full scope of the change and what it means for the dev, can we also update the README to reflect what are the commands to run now (for install deps, test, build, lint, etc...) |
c80962b
to
8c8d830
Compare
Done! I've also bumped CI to go1.9.4 (as per the linked PR with the build chain update), further decreasing CI time to around 40 seconds. |
.circleci/config.yml
Outdated
command: sudo apt-get install rake | ||
name: Install dependencies | ||
command: | | ||
go get -t ./... |
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 still use glide as we pinned configuration is glide.lock/glide.yaml.
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.
What would be the reason to do that? Everything is committed into vendor
already. Are you thinking that there is a possibility that someone commits the wrong version into vendor
and then it will cause unexpected behaviour? Because if that's the case then I think we can live with that and we should generally catch that in a review.
You can notice yourself the glide install
causes no change to the working tree. I think the general consensus is to commit the vendor
folder with any potential updates so it doesn't make sense to waste time running it every time.
Let me know if I'm missing something.
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.
vendor
isn't committed (and you won't see it with a glide install
as it is part of the .gitignore
), which is the one of the main purpose of using a tool to pin and manage dependencies.
It also allows much simpler dep management, with clear/explicit pin to specific versions of the deps.
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.
vendor
isn't committed
Is there any particular reason why we don't? I can only see positive aspects to doing so. We are already doing this on most (if not all) our projects.
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.
It is a very common practice (and is most languages, package.json
, setup.py
, Gemfile
, ...) to only commit dependency description / lock and not their code. It has plenty of benefits:
- keep the repo small, prevent non interesting diff when upgrading
- provide a way to manage your dependencies: add, remove, upgrade, lock, list, ...
Which projects are you referring to? I don't think we are committing vendors in any of them.
Now that I looked at the 2 other related PRs, I think we should still keep here and wrap the various build commands in a single command. While Rake was overkill for that, maybe we could use a simple Makefile? |
Yes, that is a good point. I have also thought about this. It keeps things more under our control. I will look into an alternative, where we can keep control on our side without having to change |
I have closed the two PRs changing the build chain. I am going to experiment with alternatives, such as creating a |
59a4d5a
to
659851a
Compare
@LotharSee as you've suggested:
PTAL |
83f5720
to
130672e
Compare
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 great to me!
Let's just be sure we update the buildchains at the same time to not impact Agent releases.
Thanks. Build chain PRs are ready. I will wait with this until GA is out to minimise any breakage risks and will merge it on Friday or Monday. |
130672e
to
2a1a251
Compare
Merging since GA was pushed to Tuesday 27th Feb. |
This change removes Rake, but before merging we must ensure that the build chain pipeline for both major agent versions will continue to work.
Related: DataDog/dd-agent-omnibus#234 and DataDog/datadog-agent#1303