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

chore: gateway-api v0.5.1 #1445

Merged
merged 6 commits into from
Nov 14, 2022
Merged

chore: gateway-api v0.5.1 #1445

merged 6 commits into from
Nov 14, 2022

Conversation

AlinsRan
Copy link
Contributor

@AlinsRan AlinsRan commented Nov 9, 2022

Type of change:

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What this PR does / why we need it:

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

Makefile Outdated

### uninstall-gateway-api: Uninstall Gateway API CRDs from the K8s cluster.
.PHONY: uninstall-gateway-api
uninstall-gateway-api: go-mod-download-gateway-api
Copy link
Member

Choose a reason for hiding this comment

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

why we need go-mod-download-gateway-api?

Copy link
Member

Choose a reason for hiding this comment

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

but its uninstall-gateway-api

Copy link
Member

Choose a reason for hiding this comment

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

Isn't it weird that I'm being asked to update it first when I want to delete it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This package does not exist when uninstalling is mainly avoided.

Copy link
Member

Choose a reason for hiding this comment

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

This is obviously not necessary.

I can get the name of the CRD in the following two ways

kubectl api-resources --api-group=gateway.networking.k8s.io -o name

or

kubectl get crds  | awk '/gateway.networking.k8s.io/ {print $1}'

Then I can get the output:

gatewayclasses.gateway.networking.k8s.io
httproutes.gateway.networking.k8s.io
referencepolicies.gateway.networking.k8s.io
tcproutes.gateway.networking.k8s.io
tlsroutes.gateway.networking.k8s.io
udproutes.gateway.networking.k8s.io

Just need a for loop, you can delete these CRD resources directly.

This does not require any dependencies, even an offline environment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not very convenient. There is also a gateway-api webhook.

Makefile Outdated

GATEWAY_API_PACKAGE ?= sigs.k8s.io/gateway-api
GATEWAY_API_VERSION ?= v0.5.1
GATEWAY_API_CRDS_LOCAL_PATH = $(shell go env GOPATH)/pkg/mod/$(GATEWAY_API_PACKAGE)@$(GATEWAY_API_VERSION)/config/crd
Copy link
Member

Choose a reason for hiding this comment

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

If we already keep a copy of this CRD configuration in the our repo, why not just use this copy to install?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dependent updates are convenient. The CRDs in the repo as an example, and maybe we can delete it?

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 keep that copy for anyone to use offline

@codecov-commenter
Copy link

Codecov Report

Merging #1445 (c459e74) into master (6f83da5) will increase coverage by 0.10%.
The diff coverage is 20.83%.

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
+ Coverage   41.25%   41.35%   +0.10%     
==========================================
  Files          82       82              
  Lines        7279     7261      -18     
==========================================
  Hits         3003     3003              
+ Misses       3929     3911      -18     
  Partials      347      347              
Impacted Files Coverage Δ
pkg/providers/gateway/translation/gateway.go 0.00% <0.00%> (ø)
pkg/providers/gateway/translation/translator.go 0.00% <ø> (ø)
...providers/gateway/translation/gateway_httproute.go 52.50% <52.63%> (ø)
pkg/providers/apisix/translation/apisix_route.go 31.77% <0.00%> (+0.35%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more


### install: Install Gateway API into the K8s cluster from go mod.
.PHONY: install-gateway-api
install-gateway-api: go-mod-download-gateway-api
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary since we have a local copy.

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

Successfully merging this pull request may close these issues.

None yet

4 participants