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

Bump gateway api version used in examples #13591

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

keithmattix
Copy link
Contributor

@keithmattix keithmattix commented Jul 18, 2023

Please provide a description for what this PR is for.
Bump gateway-api version in the examples to 0.7.1

And to help us figure out who should review this PR, please
put an X in all the areas that this PR affects.

  • Ambient
  • Docs
  • Installation
  • Networking
  • Performance and Scalability
  • Extensions and Telemetry
  • Security
  • Test and Release
  • User Experience
  • Developer Infrastructure

@keithmattix keithmattix requested review from a team as code owners July 18, 2023 17:00
@istio-testing istio-testing added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 18, 2023
dhawton
dhawton previously approved these changes Jul 18, 2023
@@ -53,7 +53,7 @@ Follow these steps to get started with ambient:

{{< text bash >}}
$ kubectl get crd gateways.gateway.networking.k8s.io &> /dev/null || \
{ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.6.1" | kubectl apply -f -; }
{ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.7.1" | kubectl apply -f -; }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are these docs using a hard coded version, instead of the latest version like all the other Istio docs?

Suggested change
{ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.7.1" | kubectl apply -f -; }
{ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref={{< k8s_gateway_api_version >}}" | kubectl apply -f -; }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess is because we want to reference a particular semver release rather than the commit SHA that currently exists. I can create another arg (e.g. k8s_gateway_api_tag) if that would be better?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Generally, we want to use the version of the Gateway API that is the same as the version that was used to build the Istio impl that we're running in the test environment. That's what {{< k8s_gateway_api_version >}} returns.

Unless I'm missing something I don't think it makes sense to not do that here.

cc @howardjohn

Copy link
Collaborator

@frankbu frankbu left a comment

Choose a reason for hiding this comment

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

See inline comment

@@ -471,5 +471,5 @@ $ kubectl delete -f @samples/sleep/notsleep.yaml@
If you installed the Gateway API CRDs, remove them:

{{< text bash >}}
$ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.6.1" | kubectl delete -f -
$ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.7.1" | kubectl delete -f -
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
$ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref=v0.7.1" | kubectl delete -f -
$ kubectl kustomize "github.com/kubernetes-sigs/gateway-api/config/crd/experimental?ref={{< k8s_gateway_api_version >}}" | kubectl delete -f -

@ericvn
Copy link
Contributor

ericvn commented Jul 18, 2023

I suspect that since we've moved to a release version in the master branch again we can revert the changes I made when it moved from release in #13313 by adding update-gateway-version back into the flow.

@ericvn
Copy link
Contributor

ericvn commented Jul 18, 2023

I did re-add the target to the make gen in #13593. Will see how the tests progress.

@ericvn
Copy link
Contributor

ericvn commented Jul 18, 2023

We also need to get the gateway-api update in istio/istio for which I created a PR as well. Once that merges, we can update istio.io to point to it, along with my other change and we will see 0.7.1 again.

@ericvn
Copy link
Contributor

ericvn commented Jul 18, 2023

Part of the equation is if we need some later commits from gateway-api for our istio repo. I see that John has a PR to pull in a later version which contains Frank's kubernetes-sigs/gateway-api#2183.

@ericvn
Copy link
Contributor

ericvn commented Jul 18, 2023

In my istio PR it is noted that 0.7.1 is a downgrade of the gateway-api version that what is being used. Since preliminary.istio.io follows the main istio branch, we may use a commit off of HEAD, with the idea that we would be back to a normal gateway-api release for our releases.

It seems that as such, we really want to stay on the current version, and likely will move up in the near future as there is an update pending in an istio PR.

One thing that isn't automated with this is we won't automatically pick the commit up in our tests/docs since the make target was disabled (hard to get actual string needed when it's not just a release). I can look into doing that again.

@frankbu
Copy link
Collaborator

frankbu commented Jul 18, 2023

@ericvn @keithmattix So IIUC, the correct fix for the docs in this PR, is to change them to use ref={{< k8s_gateway_api_version >}}, as I suggested, and the correct version will show up in the docs at some point in the near future when the related build automation is fixed up.

@keithmattix
Copy link
Contributor Author

Sounds good. Thanks @ericvn @frankbu

@dhawton dhawton dismissed their stale review July 18, 2023 20:27

changing for needed changes

Signed-off-by: Keith Mattix II <[email protected]>
@istio-testing istio-testing merged commit c7fff6c into istio:master Jul 18, 2023
@keithmattix keithmattix deleted the bump-gateway-api-version branch July 18, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants