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.

Sign up for GitHub

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

Jump to bottom

Update golang to 1.14.4 and other build deps #2480

Merged
merged 15 commits into from
Jun 17, 2020
Merged

Update golang to 1.14.4 and other build deps #2480

merged 15 commits into from
Jun 17, 2020

Conversation

gouthamve
Copy link
Contributor

Checked to see that there are no breaking changes

@gouthamve
Copy link
Contributor Author

Will merge this and open a new PR with the updated build-image

@pracucci
Copy link
Contributor

Will merge this and open a new PR with the updated build-image

@gouthamve Why? What's the problem of updating the build-image in this PR?

@bboreham bboreham mentioned this pull request Apr 18, 2020
Closed
@tomwilkie
Copy link
Contributor

@gouthamve Why? What's the problem of updating the build-image in this PR?

I think Goutham prefers to use the master-xyz tag for the build image.

@pracucci
Copy link
Contributor

@gouthamve Why? What's the problem of updating the build-image in this PR?

I think Goutham prefers to use the master-xyz tag for the build image.

I see it, but the problem is that we're going to merge a change to the build-image before seeing CI pass with it. That's why, so far, we've always updated the build-image tag along side the PR, even if the branch used for it is not master-xyz (which to me looks a secondary problem, compared to merging changes in the build-image without having the CI green light).

@pull-request-size pull-request-size bot added size/M and removed size/S labels Apr 22, 2020
@gouthamve
Copy link
Contributor Author

gouthamve commented Apr 22, 2020 •
edited

Currently blocked by etcd-io/bbolt#187 and etcd-io/bbolt#214

Hitting us both in our etcd tests (which spins up a mock etcd, hence an internal bolt instance) and our local boltdb index storage tests.

@pracucci
Copy link
Contributor

Luckily we tested before merging.

@gouthamve gouthamve mentioned this pull request Apr 27, 2020
Closed
3 tasks
This was referenced May 1, 2020
Merged
Merged
@pstibrany
Copy link
Contributor

pstibrany commented Jun 4, 2020 •
edited

Can we revisit this? Running tests locally with Go 1.14.3 works just fine for me. (I also rebased this PR on top of master, before running those tests)

@gouthamve
Update golang to 1.14.2 and other build deps
d881036 
Checked to see that there are no breaking changes

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Update the build-image image
6b350fe 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Make lint happy
bd12f90 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Upgrade nodejs as well
ad32fe2 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Update the image with node upgrade
161e809 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Make lint happy
5cc3920 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@cyriltovena cyriltovena mentioned this pull request Jun 16, 2020
Closed
@gouthamve
Update bbolt and build-image
e262ea5 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Clean up vendor
47a63df 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Bump to 1.14.4
a334435 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Update to golang 14.4
7607606 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve gouthamve changed the title Update golang to 1.14.2 and other build deps Update golang to 1.14.4 and other build deps Jun 16, 2020
@gouthamve
Update to even newer deps
ad5f9d9 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Update circle config
503edb1 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Remove mod vendor from Makefile
96d58b0 
Not required anymore

Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
pracucci
pracucci reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

Could you remove the -mod vendor from Makefile? It shouldn't be necessary anymore.

build-image/Dockerfile
RUN git clone https://github.com/gohugoio/hugo.git --branch ${HUGO_VERSION} --depth 1 && \
cd hugo && go install --tags extended && cd ../ && \
rm -rf hugo/ && rm -rf /go/pkg /go/src
RUN curl -fsSLo shfmt https://github.com/mvdan/sh/releases/download/v1.3.0/shfmt_v1.3.0_linux_amd64 && \
echo "b1925c2c405458811f0c227266402cf1868b4de529f114722c2e3a5af4ac7bb2 shfmt" | sha256sum -c && \
ENV SHFMT_VERSION=3.1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Who does use/need shfmt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will test and send another PR. Don't want to block this.

build-image/Dockerfile Outdated Show resolved Hide resolved
build-image/Dockerfile Outdated Show resolved Hide resolved
@gouthamve
Upgrade to Node 14
ce3bc5f 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
@gouthamve
Update circle config
910ed01 
Signed-off-by: Goutham Veeramachaneni <gouthamve@gmail.com>
pracucci
pracucci approved these changes Jun 17, 2020
View reviewed changes
Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

I locally tested the website with the updated Hugo version and LGTM.

@gouthamve gouthamve merged commit 29e9ce9 into cortexproject:master Jun 17, 2020
@gouthamve gouthamve deleted the update-build-image branch June 17, 2020 10:05
@pstibrany pstibrany mentioned this pull request Jun 18, 2020
Merged
@pstibrany
Copy link
Contributor

We now build binaries using Go 1.14, which is a change that should be noted in the CHANGELOG in my opinion.

@pracucci
Copy link
Contributor

We now build binaries using Go 1.14, which is a change that should be noted in the CHANGELOG in my opinion.

Mentioned in #2753.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomwilkie tomwilkie tomwilkie approved these changes

@pracucci pracucci pracucci approved these changes

Assignees
No one assigned
Labels
size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@gouthamve @pracucci @tomwilkie @pstibrany