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

plan a charm file resource built by local script #454

Merged
merged 7 commits into from
Nov 20, 2024

Conversation

addyess
Copy link
Contributor

@addyess addyess commented Oct 24, 2024

Applicable spec: KU-1646

Overview

Create/Handle integration tests plans with file resources

Rationale

Not all charms have image resources, some also use file resources. The workflows must grow the ability to plan, build, test, and publish with file resources.

Workflow Changes

The Plan, Build, Integration Test, and Publish workflows all get a bump to integrate with the new file based resources.

I didn't find any instructions on how to build the typescript so it was built with:

npm run format:write && npm run package

Checklist

  • The contributing guide was applied
  • The PR is tagged with appropriate label (urgent, trivial, complex)

I don't have the authority to set any of the tags

@addyess addyess requested a review from a team as a code owner October 24, 2024 20:44
@addyess addyess force-pushed the KU-1646/add-charm-file-resources branch 6 times, most recently from af2304d to 37b68c5 Compare October 24, 2024 21:21
@addyess addyess marked this pull request as draft October 24, 2024 21:22
@addyess addyess force-pushed the KU-1646/add-charm-file-resources branch 8 times, most recently from 64ef424 to dd35fe2 Compare October 25, 2024 13:08
@addyess addyess marked this pull request as ready for review October 25, 2024 13:18
@addyess
Copy link
Contributor Author

addyess commented Oct 25, 2024

This branch cannot be merged until the changes to the uses: <path> is returned to its original settings under .github/workflows

Copy link
Collaborator

@weiiwang01 weiiwang01 left a comment

Choose a reason for hiding this comment

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

The pull request looks solid, thanks! Though I was wondering if we might explore an alternative approach for discovering the build script. I've added some details in the inline comments. Let me know your thoughts.

README.md Outdated
@@ -28,6 +28,8 @@ The following secrets are available for this workflow:
When running the integration tests, the following posargs will be automatically passed to the `integration` target:

* --charm-file [charm_file_name]: The name of the charm artifact generated prior to the integration tests run, this argument can be supplied multiple times for charm with multiple bases.
* --{image-name}-image: The name of the image artifact built prior to the integration tests run, this argument may be supplied multiple times or not at all depending on the plan
* --{resource-name}-file-resource: The name of the charm file resources built prior to the integration tests run, this argument may be supplied multiple times or not at all depending on the plan
Copy link
Collaborator

Choose a reason for hiding this comment

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

Based on this code, should the argument name be --{resource-name}-resource?

            build.type === 'charm' ? 'charm-file' : `${name}-resource`

src/build.ts Outdated
@@ -135,6 +135,32 @@ interface BuildDockerImageParams {
token: string
}

async function buildFileResource(plan: BuildPlan): Promise<void> {
core.startGroup(`Build resource {plan.name}`)
await exec.exec('./build-resource.sh', [plan.name, plan.source_file], {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think maybe we should use multiple build scripts for different file resources. For example, if there are two file resources, foo and bar, we can create two build scripts, build-foo and build-bar, in the project directory. The operator-workflows should then run the respective build script to build the target resource.

This approach has the advantage that, currently, the source_file in the file resources build plan refers to the build target, whereas, in other build plans, it refers to the source file (charmcraft.yaml, rockcraft.yaml...) instead of the target file (charms, rocks). I assume this setup is required to pass the target file to the build-resource script as an argument. However, if we have one build script for each file resource, we can include respective build script in the build plan, making the file resources build plan more consistent with other types of artifacts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

right, i was hoping this line would generate exactly this discussion. I used build-resource.sh since i have seen this pattern in other charms which use file resources:

but in each of these above examples, it seems to closer resemble your description to have a build-{resource-name} executable.

I'm fine with this if we don't think it pollutes the working-directory with scripts.

I am still not 100% if I am fond of this solution as so many charms with file-resources may have a different resource based on the architecture or base. I have seen inconsistent ways of presenting charm file resource artifacts with respect to architecture.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I understand what you mean. For example, in flannel, if we create a build script for each resource, we would have build-flannel-amd64, build-flannel-arm64, build-flannel-s390x, which is somewhat redundant.

In this scenario, make is actually a viable option in my opinion. You can create a makefile like the one below, and operator-workflows can simply call make {resource-name} to build the desired target, such as make flannel-arm64.

ARCHS := amd64 arm64 s390x

# build charm resources
.PHONY: $(foreach arch, $(ARCHS), flannel-$(arch))
$(foreach arch, $(ARCHS), flannel-$(arch)):
	@docker run --rm -e GOARCH=$(lastword $(subst -, , $@)) ...

but I do understand that make might not be the most ergonomic to use compared to a simple bash script, especially since, ideally, the make target should be the resource filename rather than the resource name. However, different resources can have the same filename; for example, flannel-amd64 and flannel-arm64 both have the filename flannel.tar.gz, so we can't use the filename as target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@weiiwang01 ok, i went with the /build-{resource-name}.sh approach.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, @addyess. If you'd like to preserve the original build-resource.sh method, I think we can do that as well. Just add a flag to the build plan structure so the build action knows which method to apply, whether it's build-resources.sh, build-{resource-name}.sh, or make {resource-name}. This way, we give developers maximum freedom to choose the method that best suits their project.

If you want to go with multiple build methods and keep the original build-resource.sh idea, I don’t think you need to implement all of them right away. Just keep the build-resource.sh method for now, and we can add more build methods in the future.

However, if you prefer using separate scripts for different resources, I have a trick to reduce duplication. We can create a master script, for example build-flannel, which switches to different builds based on the script name. For example:

#!/bin/bash

SCRIPT_NAME=$(basename "$0")
ARCH=${SCRIPT_NAME#build-flannel-}

if [[ "$ARCH" != "amd64" && "$ARCH" != "arm64" ]]; then
  echo "error: unknown architecture '$ARCH'."
  exit 1
fi

# build based on the ARCH
GOARCH="$ARCH" go build ./...

Then, we can create individual build scripts for each resource using symbolic links:

ln -s build-flannel-amd64 build-flannel
ln -s build-flannel-arm64 build-flannel

Calling each separate build script will build the specific resource:

./build-flannel-amd64 # builds the amd64 version
./build-flannel-arm64 # builds the arm64 version

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 honestly thought about just expecting the charm metadata (via metadata.yaml or charmcraft.yaml) to provide some clue as to how to create this resource. At this moment. I think calling build-<resource>.sh from the working directory should be very clear. Our downstream projects can sort out how to make these scripts exist with symlinks or make files or whatever else.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good to me, thanks!

@addyess addyess changed the title plan a charm file resource built by script called 'build-resource.sh' plan a charm file resource built by local script Oct 29, 2024
@addyess addyess force-pushed the KU-1646/add-charm-file-resources branch 2 times, most recently from 7a7d73f to dbeaffd Compare November 12, 2024 15:24
@addyess addyess force-pushed the KU-1646/add-charm-file-resources branch from dbeaffd to 417f4e9 Compare November 12, 2024 15:28
@addyess
Copy link
Contributor Author

addyess commented Nov 14, 2024

@weiiwang01 any chance this can get an approval then?

@addyess
Copy link
Contributor Author

addyess commented Nov 18, 2024

@jdkandersson or @arturo-seijas maybe?

Copy link
Collaborator

@weiiwang01 weiiwang01 left a comment

Choose a reason for hiding this comment

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

Yes, I am ready to merge this pull request. Thank you for the significant contribution. I will force merge the pull request after the action references are changed back to canonical/operator-workflows/internal.

src/plan.ts Outdated Show resolved Hide resolved
weiiwang01
weiiwang01 previously approved these changes Nov 19, 2024
src/build.ts Outdated Show resolved Hide resolved
@addyess
Copy link
Contributor Author

addyess commented Nov 19, 2024

@weiiwang01 ready i think when you are

@weiiwang01 weiiwang01 merged commit 5c6f7a9 into canonical:main Nov 20, 2024
62 of 71 checks passed
@weiiwang01
Copy link
Collaborator

@addyess merged, thank you!

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.

2 participants