Migrate to fs.FS as the sole resource loading interface

[aslakknutsen]

What type of PR is this?

• Enhancement / New Feature
• Bug Fix
• Refactor
• Optimization
• Test
• Documentation Update

What this PR does / why we need it:

Migrate to fs.FS as the sole resource loading interface

Replace string-based ResourceDirectory with fs.FS throughout the codebase
to provide a unified abstraction for loading Helm charts and profiles.

This enables consumers to use embed.FS for bundled resources or os.DirFS
for filesystem-based resources through a single consistent interface.

[istio-testing]

Hi @aslakknutsen. Thanks for your PR.

I'm waiting for a istio-ecosystem or istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[dgn]

/ok-to-test

[aslakknutsen]

/test e2e-kind

[nrfox]

This might keep the binary smaller but the container image must still have the charts so there may not be a size difference unless there's additional overhead in embedding the charts into the binary?

[nrfox]

Do we need to read from the os? Can we embed the charts into the Sail Operator as well? Or do the charts need to be dynamic?

[aslakknutsen]

I think it's doesn't matter in the "1 build 1 container" pipeline. They will be in there either way, and if it's +-10mb doesn't really matter either.

There is a certain scenario perhaps, dev or some custom install where you might want to point to a directory to install from. I don't know if anyone is using it like that now, but it currently generally supports both modes.

If we want to go embedded with Sail Operator as well, we could use the embedded.fs if resourceDirectory is not set. It would increase the size of the binary, but allow for anyone to run external charts if needed for what ever reason.

[nrfox]

To run locally we pass --resource-dir=./resources so embedding would actually make this simpler by removing the need to pass the flag:

sail-operator/Makefile.core.mk

Lines 252 to 254 in 857518b

	.PHONY: run
	run: gen ## Run a controller from your host.
	POD_NAMESPACE=${NAMESPACE} go run ./cmd/main.go --config-file=./hack/config.properties --resource-directory=./resources

If it simplifies things to always load from embedded assets, I'm all for it. I'm not sure either if the resource dir flag is being used today in a way that wouldn't be addressed by embedding. If we want to keep it in case someone is using it this way then we could default to reading from embedded assets and only read from the filesystem if it's specified? Or we could remove the flag altogether and only add back if someone requests it.

[aslakknutsen]

I've changed the logic to swap to embedded.FS if resourceDirectory argument is missing.

Also removed the resourceDirectory reference from the Makefile and Dockerfile. Not sure if there are any other locations that would need updating as well, e.g. downstream images?

de860cb

[nrfox]

There's a helm issue to support this. Maybe long term we can get this added into helm itself.

[nrfox]

Tests for this?

[aslakknutsen]

https://github.com/istio-ecosystem/sail-operator/pull/1561/changes#diff-3a24081c87e2f7f283c1870606d48dc0d23c23511fadcf17ead0a0e9ecb8103b

[aslakknutsen]

/test docs-test

[dgn]

/cherry-pick release-1.28

[istio-testing]

@dgn: new pull request created: #1594

Details

In response to this:

/cherry-pick release-1.28

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[istio-testing]

@dgn: new pull request could not be created: failed to create pull request against istio-ecosystem/sail-operator#release-1.28 from head istio-testing:cherry-pick-1561-to-release-1.28: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for istio-testing:cherry-pick-1561-to-release-1.28."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request","status":"422"}

Details

In response to this:

/cherry-pick release-1.28

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.