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

Register plug-in as Packer integration #116

Merged
merged 13 commits into from
Feb 12, 2024

Conversation

nywilken
Copy link
Contributor

👋 fellow Packer maintainer working on migrating your plugin to the Packer integration framework.

This change takes the necessary steps to register this plugin as an official Packer integration.
Integrations can be found on the Packer integration portal at https://developer.hashicorp.com/packer/integrations.


The pull-request consists of the following changes:

  • Adds controlling, metadata file, metadata.hcl for registering the plug-in and it components as integrations.
    Details on the contents, along with a description of the attributes, can be found at https://github.com/hashicorp/integration-template.
  • Adds the GitHub action workflows for triggering manual and automatic integration updates.
  • Restructures the plug-in documentation to match the expected format of the integration framework.
  • Adds a .web-docs directory for serving the fully render documentation as the integration docs.
  • Adds the build-docs make target make build-docs for syncing changes to the docs directory to the .web-docs directory.

Changes to the integration docs can be made at plugin release via the notify-integration-release-via-tag workflow or
manually by running the notify-integration-release-via-manual workflow.

Details on how the Integration framework pipeline works can be found at hashicorp/packer#12702

TODOs

  • Open pull-request against external plugin.
  • Update integration description .web-docs/metadata.hcl.
  • Packer team open internal pull-request to enable integration.
  • Review plugin integration on Packer integration portal .... Iterate

@bentranter
Copy link
Member

Hey @nywilken, thanks for this! Most of our team is out until Monday for American Thanksgiving, so we'll have a look at this then.

.web-docs/metadata.hcl Outdated Show resolved Hide resolved
@nywilken
Copy link
Contributor Author

Thank you @bentranter. There are a few small updates that need to be made to complete the PR but please feel free to review the docs when you or the team have time. lbajolet-hashicorp will be assisting in getting this ready over the next couple of days.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

Thanks for getting this all together for us! I did have a few small suggestions and comments inline.

.github/workflows/ensure-docs-compiled.yaml Outdated Show resolved Hide resolved
Comment on lines +3 to +5
push:
tags:
- '*.*.*' # Proper releases
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
push:
tags:
- '*.*.*' # Proper releases
push:
tags:
- 'v*.*.*' # Proper releases

We only initiate releases when there is a leading v in the tag. We should probably follow the same practice here.

That also raises a question for me about timing. If both our release workflow and the notify release are triggered by pushing a tag, it seems like we may notify you of the release before it has actually completed. Are there any concerns there? Would using the release trigger make sense here?

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#release

Copy link
Contributor

Choose a reason for hiding this comment

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

Prefixing with a v looks good to me, we could probably adopt this for our plugins as well, though the regex does catch it as-is, hence the step to remove the leading v from the tag (the doc servers reject requests for versions with a trailing v).

For timing, we also do it in this order, and you're correct that the docs will be signalled as needing update before the release finished. In practice we haven't experienced problems (we scrape the docs from the tag, so the commit already exists and we can fetch the data), but we could indeed end-up with a release advertised on the docs site, even if the actual release process failed for some reason. Running this after the release has completed is probably a better idea indeed, and something we should do as well, thanks for the heads-up.

Copy link
Member

Choose a reason for hiding this comment

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

Great. Thanks for looking into this. Let us know how the testing goes!

Copy link
Contributor

@lbajolet-hashicorp lbajolet-hashicorp left a comment

Choose a reason for hiding this comment

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

Hi @andrewsomething,

Thanks for the review, and sorry we took this long to respond.

I've responded to the comments you made, they're all good points. I'll update the actions to reuse your makefile check today, as for the trigger for the release, I'd like to experiment with our repos before attempting to change your configs (testing workflows is notoriously unreliable, especially those that aren't triggered on the PR).

I'll follow up with an update ASAP

.github/workflows/ensure-docs-compiled.yaml Outdated Show resolved Hide resolved
Comment on lines +3 to +5
push:
tags:
- '*.*.*' # Proper releases
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefixing with a v looks good to me, we could probably adopt this for our plugins as well, though the regex does catch it as-is, hence the step to remove the leading v from the tag (the doc servers reject requests for versions with a trailing v).

For timing, we also do it in this order, and you're correct that the docs will be signalled as needing update before the release finished. In practice we haven't experienced problems (we scrape the docs from the tag, so the commit already exists and we can fetch the data), but we could indeed end-up with a release advertised on the docs site, even if the actual release process failed for some reason. Running this after the release has completed is probably a better idea indeed, and something we should do as well, thanks for the heads-up.

Since every PR that gets merged has a check performed to make sure the
docs are up-to-date, we don't need to add an extra action that does the
same thing differently.
Since the Makefile has a target for generating and checking that there's
no diff between what's been generated and what's versioned, we don't
need custom logic to perform the check, so we reuse this target for that
step.
While updating the docs, the README forgot to be regenerated before
being added to the PR, causing the checks to fail (expectedly).
This commit updates the .web-docs/README so they match those in docs.
@lbajolet-hashicorp
Copy link
Contributor

Hi @andrewsomething,

Pinging on this issue, sorry I couldn't get to updating the action beforehand, I've been busy on other things recently. I've added a ticket on our internal boards to keep track of this, and have scheduled work on it in the coming weeks.

In the meantime, may I ask if you're able to merge this? We have migrated over to the new integrations system recently, so as of now the documentation for this plugin is not available anymore on our documentation site, and we've received reports (for example this issue) that users can't access them anymore.

If you prefer not to merge the action for publishing automatically on release because of the ordering issue, please feel free to remove it from the repo for now, we'll open a PR with it once we've fixed the scheduling problem.

Copy link
Member

@andrewsomething andrewsomething left a comment

Choose a reason for hiding this comment

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

👍 Sounds good @lbajolet-hashicorp

Thanks again!

@andrewsomething andrewsomething merged commit 601c064 into digitalocean:main Feb 12, 2024
7 checks passed
@andrewsomething
Copy link
Member

andrewsomething commented Feb 12, 2024

@lbajolet-hashicorp After mergining, I attempted to run the manual action for our latest release, v1.2.2. It failed with:

Error: Failed to notify release: API responded with [400]

Are there any additional steps we need to take to get the docs back up?

https://github.com/digitalocean/packer-plugin-digitalocean/actions/runs/7876258811/job/21489890317#step:6:57

@lbajolet-hashicorp
Copy link
Contributor

Well that is my bad, it looks like we didn't include the strip-version that removes the leading v in front of the version number, and our APIs reject that kind of version name, hence the 400 you received.
I'll open a PR to fix that right now, sorry about that!

@lbajolet-hashicorp
Copy link
Contributor

Oh looks like this is how we implemented it for all our plugins, without the strip-version job, presumably because we assumed we'd call it without the leading v in it. In retrospect though, it is probably better to make sure it isn't, so I'll see to test this in another repo, and I'll push that to every plugin we know of.
In the meantime, you will be able to call it with 1.2.2 as argument instead of v1.2.2, and that should succeed.

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.

4 participants