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

[WIP] Generic provider template #1140

Draft
wants to merge 15 commits into
base: master
Choose a base branch
from
Draft

[WIP] Generic provider template #1140

wants to merge 15 commits into from

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Nov 12, 2024

This is a very WIP fork of our bridged workflows which is intended to be used by native, component, or other miscellaneous providers not currently managed by ci-mgmt.

There are two primary design principles which deviate from the bridged template:

  1. make targets define the entire contract between a provider and ci-mgmt. We do not assume anything about the provider, e.g. it could be implemented in a language other than Go.
  2. The provider is responsible for defining its own overrides and customizations, and ci-mgmt is responsible for exposing the appropriate hooks (Allow make target customization #1131).

For example, if the provider needs some additional tooling then it is responsible for ensuring the tool is installed. Historically these sorts of customizations have been handled as one-offs in ci-mgmt, but we want to push this down to the make target or into the test itself. Not only does this make ci-mgmt easier to maintain and refactor, but it also forces us to write tests that are more easily reproducible in local environments. The SSH key handling between the docker and docker-build providers is illustrative.

I've been testing this in EKS and haven't gotten tests passing reliably yet. I was planning on consolidating the test into a shared action once I got that far, but also feel free to do that sooner than later!

EKS was already sharding tests so I've opted to keep that behavior here -- a provider just needs to implement a test_shard target which takes a TESTS and TAG env var. I've tweaked it somewhat so the sharding is totally arbitrary, which in practice just means we need to install all the SDKs instead of one at a time.

Other differences:

TODO:

  • Make e2e test path configurable (currently hard-coded to examples).
  • Make shard determination configurable and local (currently a workflow step).
  • Consolidate test action.

@blampe blampe marked this pull request as draft November 12, 2024 00:14
@blampe blampe changed the base branch from master to blampe/typed-config November 12, 2024 00:15
Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

Direction & progress looks good. It would probably be worth writing down a design for the required makefile targets and artifacts while doing this work - perhaps in a README somewhere in this repo - which can serve as a design doc too.

Comment on lines +52 to +59
- name: Download schema-embed.json
uses: #{{ .Config.ActionVersions.DownloadArtifact }}#
with:
# Use a pattern to avoid failing if the artifact doesn't exist
pattern: schema-embed.*
# Avoid creating directories for each artifact
merge-multiple: true
path: provider/cmd/pulumi-resource-#{{ .Config.Provider }}#/schema-embed.json
Copy link
Member

Choose a reason for hiding this comment

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

Not all providers use schema-embed right now. It's only realy a performance optimisation which could be implemented internally within the provider rather than needing CI to have insight of.

We mostly still have the original schema commited (though this has been taken out Azure Native due to the file being too big for GitHub, and LFS causing a load of other problems) but I think it would be more correct to treat the schema like a build artifact as you're doing here, though it would be nicer to not assume the Go provider directory structure if possible within the CI job. I think this could be abstracted by the makefile if needed.

  1. Prerequisites writes all artifacts to the bin directory including the embeddable schema if it needs it.
  2. Restore the bin directory in this subsequent job.
  3. Within the provider_dist target, copy the embeddable schema into the provider directory before building.

merge-multiple: true
path: provider/cmd/pulumi-resource-#{{ .Config.Provider }}#/schema-embed.json
- name: Build & package provider
run: make provider_dist-${{ matrix.platform.os }}-${{ matrix.platform.arch }}
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should use the file target here to match the path we're going to upload rather than having the assumption of the path this phony target will write to:

make dist/pulumi-resource-#{{ .Config.Provider }}#-v${{ inputs.version }}-${{ matrix.platform.os }}-${{ matrix.platform.arch }}.tar.gz

Comment on lines +52 to +53
- name: Install plugins
run: make install_plugins
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 a bit of an odd step. I know for TF providers it's related to installing other bridged TF providers which the bridge will use for examples generation, but for a generic template we might want something a little more generic such as make prepare or something that allways runs at the start of the job before restoring any targets or doing make --touch ... etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was also wondering about this because the CLI should automatically download these plugins anyway. Related - these plugins eat a ton of space! pulumi/pulumi#17718

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 for bridged providers, if they're opted into using the converter plugin, then it explicitly disables auto-download because we need to have the provider version pinned, so failing fast is good. For generic providers, perhaps we can just let the provider figure out what it needs and how it will install them internally within the makefile.

# otherwise use the current SHA for any other type of build.
sha: ${{ github.event.pull_request.head.sha || github.sha }}

# TODO: Extract into shared action.
Copy link
Member

Choose a reason for hiding this comment

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

👍

@@ -0,0 +1,48 @@
# WARNING: This file is autogenerated - changes will be overwritten if not made via https://github.com/pulumi/ci-mgmt

name: license_check
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 this should already be included from the provider template .. unless there's something subtle we're tweaking here I didn't spot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I duplicated it here because of #1133

Comment on lines +96 to +97
case "generic":
return []string{"provider", "pulumi-provider", "generic"}, nil
Copy link
Member

Choose a reason for hiding this comment

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

When we use this template for the native boilerplate, we'll probably add external-generic which will exclude the pulumi-provider sub-template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe "third-party"?

Copy link
Member

Choose a reason for hiding this comment

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

Just above is the external-bridged-provider template which is like bridged-provider but with bits removed for third parties. Whatever name we use should aim to keep these consistent.

Base automatically changed from blampe/typed-config to master November 15, 2024 21:41
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