Skip to content

Fix issues identified during API review#254

Merged
istio-testing merged 3 commits intoistio-ecosystem:mainfrom
luksa:update-api
Aug 9, 2024
Merged

Fix issues identified during API review#254
istio-testing merged 3 commits intoistio-ecosystem:mainfrom
luksa:update-api

Conversation

@luksa
Copy link
Copy Markdown
Contributor

@luksa luksa commented Aug 8, 2024

No description provided.

@luksa luksa requested a review from a team as a code owner August 8, 2024 13:47
nrfox
nrfox reviewed Aug 8, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@nrfox nrfox left a comment

Choose a reason for hiding this comment

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

LGTM all issues I found are fixed

@luksa
Copy link
Copy Markdown
Contributor Author

luksa commented Aug 8, 2024

/retest

dgn
dgn approved these changes Aug 9, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dgn dgn left a comment

Choose a reason for hiding this comment

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

lgtm except for one question. Adding hold in case you want to make further changes, feel free to remove

/hold

chart/crds/operator.istio.io_istiocnis.yaml Outdated
enabled:
description: Controls whether ambient redirection is enabled
type: boolean
ipv6:
Copy link
Copy Markdown
Collaborator

@dgn dgn Aug 9, 2024

Choose a reason for hiding this comment

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

hmm. as we don't support ambient yet, maybe we want to remove this for now?

Copy link
Copy Markdown
Contributor Author

@luksa luksa Aug 9, 2024

Choose a reason for hiding this comment

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

Removed. Thanks.

@dgn
Copy link
Copy Markdown
Collaborator

dgn commented Aug 9, 2024

just noticed that this still contains the autoscalingv2 field:

sail-operator/api/v1alpha1/values_types.gen.go

Lines 410 to 412 in ac99ae0

// TODO: remove this?
// No longer used.
Autoscalingv2API *bool `json:"autoscalingv2API,omitempty"`

I can submit a patch to remove it upstream and then update the commit we use in a separate PR

@luksa
Copy link
Copy Markdown
Contributor Author

luksa commented Aug 9, 2024

just noticed that this still contains the autoscalingv2 field:
I can submit a patch to remove it upstream and then update the commit we use in a separate PR

Go ahead and remove it from upstream. But you don't need to update the commit, because I'm working on making the API transformer use the commits in go.mod, so your changes will automatically be pulled in when istio is updated.

luksa added 3 commits August 9, 2024 11:07
@luksa
Fix issues identified during API review
91bff35 
Signed-off-by: Marko Lukša <mluksa@redhat.com>
@luksa
Pull in latest upstream changes
0a7ee1b 
Signed-off-by: Marko Lukša <mluksa@redhat.com>
@luksa
Remove CNIConfig.Ambient
dc398c4 
Signed-off-by: Marko Lukša <mluksa@redhat.com>
@luksa
Copy link
Copy Markdown
Contributor Author

luksa commented Aug 9, 2024

/hold cancel

@istio-testing istio-testing merged commit af32c9e into istio-ecosystem:main Aug 9, 2024
@luksa luksa deleted the update-api branch August 9, 2024 09:37
dgn pushed a commit to dgn/sail-operator that referenced this pull request Aug 13, 2024
@luksa @dgn
Fix issues identified during API review (istio-ecosystem#254)
6887ae9 
* Fix issues identified during API review

Signed-off-by: Marko Lukša <mluksa@redhat.com>

* Pull in latest upstream changes

Signed-off-by: Marko Lukša <mluksa@redhat.com>

* Remove CNIConfig.Ambient

Signed-off-by: Marko Lukša <mluksa@redhat.com>

---------

Signed-off-by: Marko Lukša <mluksa@redhat.com>
Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
openshift-service-mesh-bot referenced this pull request in openshift-service-mesh-bot/sail-operator Aug 13, 2024
Automated merge
6e4cac8 
* upstream/main:
  Ensure `make gen-check` works when go mod cache is empty (openshift-service-mesh#270)
  Introduce RemoteIstio (openshift-service-mesh#202)
  api_transformer: use `go env` to detect mod cache dir (openshift-service-mesh#269)
  Add field values.experimental to API (openshift-service-mesh#264)
  Fix errors in tables for api reference documentation (openshift-service-mesh#263)
  Generate CNIGlobalConfig from GlobalConfig automatically (openshift-service-mesh#260)
  Fix errors in api reference doc (openshift-service-mesh#262)
  Rename `addTags` to `addComments` in API transformer (openshift-service-mesh#259)
  Add relatedImages to ClusterServiceVersion (openshift-service-mesh#243)
  Generate API from versions defined in go.mod (openshift-service-mesh#258)
  Fix issues identified during API review (openshift-service-mesh#254)

# Conflicts:
#	bundle/manifests/sailoperator.clusterserviceversion.yaml
istio-testing pushed a commit that referenced this pull request Aug 13, 2024
@luksa @istio-testing
Fix issues identified during API review (#254)
bb28997 
* Fix issues identified during API review

Signed-off-by: Marko Lukša <mluksa@redhat.com>

* Pull in latest upstream changes

Signed-off-by: Marko Lukša <mluksa@redhat.com>

* Remove CNIConfig.Ambient

Signed-off-by: Marko Lukša <mluksa@redhat.com>

---------

Signed-off-by: Marko Lukša <mluksa@redhat.com>
Signed-off-by: Daniel Grimm <dgrimm@redhat.com>
openshift-service-mesh-bot referenced this pull request in openshift-service-mesh-bot/sail-operator Aug 13, 2024
Automated merge
187498d 
* upstream/main:
  Add `RemoteIstio` to API concepts in docs (openshift-service-mesh#272)
  Ensure `make gen-check` works when go mod cache is empty (openshift-service-mesh#270)
  Introduce RemoteIstio (openshift-service-mesh#202)
  api_transformer: use `go env` to detect mod cache dir (openshift-service-mesh#269)
  Add field values.experimental to API (openshift-service-mesh#264)
  Fix errors in tables for api reference documentation (openshift-service-mesh#263)
  Generate CNIGlobalConfig from GlobalConfig automatically (openshift-service-mesh#260)
  Fix errors in api reference doc (openshift-service-mesh#262)
  Rename `addTags` to `addComments` in API transformer (openshift-service-mesh#259)
  Add relatedImages to ClusterServiceVersion (openshift-service-mesh#243)
  Generate API from versions defined in go.mod (openshift-service-mesh#258)
  Fix issues identified during API review (openshift-service-mesh#254)

# Conflicts:
#	bundle/manifests/sailoperator.clusterserviceversion.yaml
dgn pushed a commit to dgn/sail-operator that referenced this pull request Feb 25, 2025
@luksa
Ensure e2e tests use the correct versions yaml file (istio-ecosystem#667
9699702 
) (istio-ecosystem#669) (istio-ecosystem#254)

We use `-ldflags` to set the `istioversion.versionsFilename` variable, but this only works when building the binary. It doesn't work when running ginkgo-based e2e tests. When the `versionsFilename` is not set via ldflags, it will now use the environment variable `VERSIONS_YAML_FILE`. If this env var isn't set, we default to `versions.yaml`.

(cherry picked from commit 148eb64)

Signed-off-by: Marko Lukša <mluksa@redhat.com>
yannuil pushed a commit to yannuil/sail-operator that referenced this pull request Jun 20, 2025
@luksa @yannuil
Ensure e2e tests use the correct versions yaml file (istio-ecosystem#667
1d21cb5 
) (istio-ecosystem#669) (istio-ecosystem#254)

We use `-ldflags` to set the `istioversion.versionsFilename` variable, but this only works when building the binary. It doesn't work when running ginkgo-based e2e tests. When the `versionsFilename` is not set via ldflags, it will now use the environment variable `VERSIONS_YAML_FILE`. If this env var isn't set, we default to `versions.yaml`.

(cherry picked from commit 148eb64)

Signed-off-by: Marko Lukša <mluksa@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nrfox nrfox nrfox left review comments

@dgn dgn dgn approved these changes

Assignees

No one assigned

Labels

size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@luksa @dgn @nrfox @istio-testing