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

Setup dependabot for go dependencies #1754

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

CharlieTLe
Copy link

@CharlieTLe CharlieTLe commented Oct 27, 2024

Changes

Sets up dependabot to check for updates on Go dependencies.

Also, the pb.go files were checked in to resolve the errors from Dependabot:

go: github.com/open-telemetry/opentelemetry-demo/src/checkoutservice imports
	github.com/open-telemetry/opentelemetry-demo/src/checkoutservice/genproto/oteldemo: cannot find module providing package github.com/open-telemetry/opentelemetry-demo/src/checkoutservice/genproto/oteldemo

Merge Requirements

For new features contributions, please make sure you have completed the following
essential items:

  • CHANGELOG.md updated to document new feature additions
  • Appropriate documentation updates in the docs
  • Appropriate Helm chart updates in the helm-charts

Maintainers will not merge until the above have been completed. If you're unsure
which docs need to be changed ping the
@open-telemetry/demo-approvers.

@CharlieTLe CharlieTLe requested a review from a team as a code owner October 27, 2024 01:59
@@ -0,0 +1,2739 @@
// Copyright 2020 Google LLC
Copy link
Member

@julianocosta89 julianocosta89 Oct 28, 2024

Choose a reason for hiding this comment

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

the proto files should not be committed.
They are built within the dockerfile:
https://github.com/open-telemetry/opentelemetry-demo/blob/main/src/checkoutservice/Dockerfile#L23

Copy link
Author

Choose a reason for hiding this comment

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

The pb.go files were checked in to resolve the errors from Dependabot:

go: github.com/open-telemetry/opentelemetry-demo/src/checkoutservice imports
github.com/open-telemetry/opentelemetry-demo/src/checkoutservice/genproto/oteldemo: cannot find module providing package github.com/open-telemetry/opentelemetry-demo/src/checkoutservice/genproto/oteldemo

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 we should find another way of solving the dependabot issue.
The main idea is to not have to touch this file if the pb changes.

Copy link
Author

Choose a reason for hiding this comment

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

@MrAlias @dashpole any ideas on a workaround for using dependabot without checking in pb.go files?

Copy link

Choose a reason for hiding this comment

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

Hmm, I'm not sure how to make this work without checking in all the Go source files that the package depends on. There is no GitHub action that I know you can hook into prior to the dependabot one runs.

FWIW, we commit the generated proto Go code in other repositories when it is used:

And so do other projects:

It is a pretty standard practice to have all the Go code, generated or not, that comprises a package checked in to the used VCS.

Copy link

Choose a reason for hiding this comment

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

@julianocosta89 can you link to the dependabot action definition? I thought that was completely automated?

Copy link
Author

Choose a reason for hiding this comment

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

Seems like there is an open issue for this: dependabot/dependabot-core#9638

Copy link
Member

Choose a reason for hiding this comment

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

🤔 maybe I'm totally lost here, but isn't that exactly what we are updating in this PR?
.github/dependabot.yml would be the file.
Before running the dependabot action, can't we call another action?

Copy link
Contributor

Choose a reason for hiding this comment

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

no, it cant execute workflows. I agree with OP that committing generated code is a good path forward and actually makes change explicit.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do this, we need to commit generated code for all languages/services. I don't want to have special cases for specific languages or services in the demo.

@puckpuck
Copy link
Contributor

We chatted about this at the last SIG meeting. Since this PR requires the generated protobuf code to be part of the checked-in files, we want to take a step back and look at this effort holistically for all services in the demo.

To keep everything consistent across all languages and services, we will start an effort to get the generated protobuf files done for all services. This is more than just generating and including the protobuf files; we also need to remove any related steps in the Dockerfile that would generate this code.

@julianocosta89
Copy link
Member

julianocosta89 commented Nov 17, 2024

We chatted about this at the last SIG meeting. Since this PR requires the generated protobuf code to be part of the checked-in files, we want to take a step back and look at this effort holistically for all services in the demo.

To keep everything consistent across all languages and services, we will start an effort to get the generated protobuf files done for all services. This is more than just generating and including the protobuf files; we also need to remove any related steps in the Dockerfile that would generate this code.

When the last SIG meeting happened?! 🤔
I joined last Wednesday but nobody was in the call.

Anyways, I agree and I'd add another step.

We need to also add a GH action to validate if the generated proto files are the expected ones based on the proto we have.

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.

5 participants