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

Add go build CI job #78

Merged
merged 5 commits into from
Nov 27, 2023
Merged

Add go build CI job #78

merged 5 commits into from
Nov 27, 2023

Conversation

sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Nov 20, 2023

One of the recent PRs broke go users: #76. This PR is to address this type of error by adding cd go && go build ./... step to the CI.

Major changes:

Fixes #77.
Closes #67, #18.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
go/go.mod Outdated
@@ -1,9 +1,17 @@
module github.com/cncf/xds/go

go 1.11
go 1.20

Choose a reason for hiding this comment

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

can bump to go 1.21

BTW, add a go mod verify

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 think I was wrong to bump it that high in the first place. This is the minimal version of go required to use this module: https://go.dev/ref/mod#go-mod-file-go

So projects like grpc won't be able to use this package because they declare lower minimal version, f.e. 1.19 in gRPC's case: https://github.com/grpc/grpc-go/blob/287c47355e154ac2193bf743c886656cd48e753c/go.mod#L3

I think we should set it to something reasonable, higher than 1.11, but lower than 1.20.

Choose a reason for hiding this comment

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

Should this be set to the version of grpc (1.19) then?

.gitignore Show resolved Hide resolved
ci/azure-pipelines.yml Outdated Show resolved Hide resolved
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk marked this pull request as ready for review November 21, 2023 21:24
@sergiitk
Copy link
Contributor Author

This is ready for review now.

@sergiitk
Copy link
Contributor Author

Just FYI - since this changes the minimal required version of the go module, we shouldn't merge it during the Thanksgiving week 😄

@adisuissa
Copy link

Just FYI - since this changes the minimal required version of the go module, we shouldn't merge it during the Thanksgiving week 😄

Good idea :)
Feel free to ping me next week if I forget to merge this.

@sergiitk
Copy link
Contributor Author

@adisuissa Good to go now.

Copy link

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@adisuissa adisuissa merged commit 8e57aac into cncf:main Nov 27, 2023
4 checks passed
@sergiitk sergiitk deleted the ci-go branch November 27, 2023 18:23
sergiitk added a commit to sergiitk/xds that referenced this pull request Nov 27, 2023
…ncf#79)"

This reverts commit 5b9bca5.

Reverts cncf#79.
Now that cncf#78 `go build` CI is in place; this PR brings back the
CEL fields.

Once `del.dev` domain is fixed, we'll see the CI passing and will be
able to merge this PR.

Original comment
---

This PR adds canonical CEL
(https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields
to `xds.type.v3.CelExpression`. Canonical CEL `cel.expr` was created
identical to the `google.api.expr.v1alpha1`, but may be extended in a
backward-compatible way.

Nuances: 1. The new fields `cel_expr_parsed` and `cel_expr_checked` are
added outside of `oneof expr_specifier` per updated policy change:
envoyproxy/envoy#30851 2. `option
(validate.required) = true` is removed from the `oneof expr_specifier`,
so the users may not presume one of the `parsed_expr`, `checked_expr`
will be set.

---

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
sergiitk added a commit to sergiitk/xds that referenced this pull request Jan 24, 2024
…ncf#79)"

This reverts commit 5b9bca5.

Reverts cncf#79.
Now that cncf#78 `go build` CI is in place; this PR brings back the
CEL fields.

Once `del.dev` domain is fixed, we'll see the CI passing and will be
able to merge this PR.

Original comment

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
---

This PR adds canonical CEL
(https://github.com/google/cel-spec/tree/master/proto/cel/expr) fields
to `xds.type.v3.CelExpression`. Canonical CEL `cel.expr` was created
identical to the `google.api.expr.v1alpha1`, but may be extended in a
backward-compatible way.

Nuances: 1. The new fields `cel_expr_parsed` and `cel_expr_checked` are
added outside of `oneof expr_specifier` per updated policy change:
envoyproxy/envoy#30851 2. `option
(validate.required) = true` is removed from the `oneof expr_specifier`,
so the users may not presume one of the `parsed_expr`, `checked_expr`
will be set.

---

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
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.

CI should perform go build Please upgrade go/go.mod grpc package
3 participants