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

Fix compiled logproto protobuf #1183

Merged
merged 5 commits into from
Oct 24, 2019

Conversation

pracucci
Copy link
Contributor

@pracucci pracucci commented Oct 19, 2019

What this PR does / why we need it:

The PR #1133 has upgraded golangci-lint and thus the loki-build-image has been rebuilded and a new version published (0.7.3).

The rebuild of the image has installed the latest versions of the deps specified in the loki-build-image/Dockerfile, including protoc-gen-gogoslick which I think is the cause of a change to pkg/logproto/logproto.pb.go.

We didn't notice it, because in the Makefile the %.pb.go target is not a phony one, so the logproto.proto is recompiled only once its timestamp change. However, it should be recompiled also whenever the tooling change and it may be quite difficult to remember it, so I'm proposing to enforce it when running make check-generated-files.

Unfortunately, the only idea I've got to enforce protoc whenever we run make check-generated-files is a bit hacky. I'm open to better ideas.

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Makefile Show resolved Hide resolved
@joe-elliott
Copy link
Member

I like the idea of always force checking on protos generation and I'm not good enough with Makefiles to suggest anything better. Generally, I like this change.

I am, however, concerned about changing logproto.pb.go this close to releasing 0.4.0. Should we pin the tooling in the Dockerfile instead and merge the updated logproto.pb.go after cutting 0.4.0?

@pracucci
Copy link
Contributor Author

Should we pin the tooling in the Dockerfile instead [...]?

@joe-elliott You mean pin the version of the following packages in loki-build-image/Dockerfile, release a new version of loki-build-image, upgrade to the new image and revert the changes to logproto.pb.go?

  • github.com/golang/protobuf/protoc-gen-go
  • github.com/gogo/protobuf/protoc-gen-gogoslick
  • github.com/gogo/protobuf/gogoproto

@joe-elliott
Copy link
Member

joe-elliott commented Oct 21, 2019

Yes, that feels safer to me, or just merge this change after 0.4.0 is cut?

@slim-bean Thoughts either way?

Recent changes to the build image have caused inconsistency in our build pipeline outlined by marco above.

Options are:

  • merge his fix now which includes changes to logproto.pb.go due to a version change in protobuf tooling
  • pin the protobuf tooling for now to make CI more consistent and keep logproto.pb.go unchanged
  • wait til after 0.4.0 and merge, but CI might be a bit flaky until then.

@slim-bean
Copy link
Collaborator

I like the idea of pinning the versions of golang and gogo protobuf and making a new build image 👍

@pracucci
Copy link
Contributor Author

@joe-elliott @slim-bean May you check the changes, please? I've already advanced the loki-build-image version even if has not been published yet (I've tested it locally). If we're good with it, is there any automation to build and publish loki-build-image:0.7.4? (I've seen make loki-build-image tags it with the current branch name).

Copy link
Collaborator

@slim-bean slim-bean left a comment

Choose a reason for hiding this comment

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

LGTM, one question/thought if we should also pin the yacc version

loki-build-image/Dockerfile Outdated Show resolved Hide resolved
@pracucci
Copy link
Contributor Author

@slim-bean I've pushed the loki-build-image:0.7.4 and added a Makefile target to do it. The CI build passes. If you don't see any issue, should be ready to merge.

@slim-bean slim-bean merged commit de026a0 into grafana:master Oct 24, 2019
@pracucci pracucci deleted the fix-logproto-compiling branch October 24, 2019 15:32
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.

3 participants