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

Final Comment: Automated build and submit pipeline #17

Merged
merged 8 commits into from
May 31, 2022

Conversation

philpax
Copy link
Contributor

@philpax philpax commented May 3, 2022

First DIP, yay!

This is a first draft with a few unanswered questions. I've left them vague so that we can specify them.

Would like input from @ayyaruq and @NotAdam if possible.

@philpax philpax requested review from goaaats and kalilistic May 3, 2022 08:36
@philpax philpax changed the title Add automated build and submit pipeline DIP WIP: Automated build and submit pipeline May 3, 2022
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
@NotNite

This comment was marked as resolved.

@NotNite

This comment was marked as resolved.

@KazWolfe

This comment was marked as resolved.

@karashiiro

This comment was marked as resolved.

@karashiiro

This comment was marked as resolved.

@kalilistic

This comment was marked as resolved.

@KazWolfe

This comment was marked as resolved.

@philpax

This comment was marked as outdated.

@philpax philpax mentioned this pull request May 6, 2022
@philpax philpax requested a review from kalilistic May 7, 2022 01:37
@philpax

This comment was marked as outdated.

@kalilistic
Copy link

Should probably add @daemitus to this one too given his work on the existing github action.

@KazWolfe
Copy link
Member

KazWolfe commented May 7, 2022

As part of this process, we should add Actions to the PR process to verify that:

  1. The plugin actually builds properly.
  2. The resulting manifest contains all required fields (e.g. name, punchline, version)
  3. The plugin passes a basic automated test suite (containing what? Signature validation? Security?)

@karashiiro
Copy link
Contributor

Oh yeah, I'm not sure if it's useful or not but I just remembered I had this https://github.com/karashiiro/Dalamud-Checksummer/blob/main/Dalamud-Checksummer.ps1 that I was working on a while back.

Copy link

@ayyaruq ayyaruq left a comment

Choose a reason for hiding this comment

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

Overall looks good, I have some nits with the user experience, and I think there's over-emphasis on things that are't fully defined. There's also some customisation decisions that don't make sense for a build service.

While I agree that a decentralised approach makes the most sense, I'm not sure forcing everything into a goatcorp repo is the way to do that. Consider a GHA plugin that can run directly in user repos and submits a manifest for review. This way the free build hours aren't shared across all users, you get more build concurrency, and if the actions are well made you can still enforce all the checking.

A separate "CD" process or whatever could then check the actual code before making the manifest public to users. I'd likely proxy the URLs to GH with a perma redirect tho, so users hit the CF fronted web server for the manifest and then download it from GH releases instead of wasting goatcorp bandwidth. The downside here is your GHA's need to be better, and it's slightly more iffy for non-GH users that would need some goatcorp repo to use instead. I expect some hybrid is likely the actual answer, or otherwise centralise it and try find a way to make builds not cost a fortune. More repos is likely not the answer tho, especially if it involves manually PRing what's essentially a version string bump. Just automate the PR outright.

text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
text/0000-automated-build-and-submit-pipeline.md Outdated Show resolved Hide resolved
@ayyaruq
Copy link

ayyaruq commented May 10, 2022

I'll make a note about making sure the action is compatible for local running...

Generally stuff that relies on GitHub specific APIs (like cache) doesn't work in act, anything else should be gucci.

@kalilistic
Copy link

Maybe I got excited and approved early. Looks like this is pending some cleanup by @philpax based on the additional comments.

@philpax philpax requested a review from kalilistic May 24, 2022 13:11
@philpax
Copy link
Contributor Author

philpax commented May 24, 2022

Just updated with latest round of feedback. Instituting informal final comment period closure in a week's time, so the 31st. Get your feedback in now!

@philpax philpax mentioned this pull request May 24, 2022
@karashiiro

This comment was marked as resolved.

@philpax philpax changed the title WIP: Automated build and submit pipeline Final Comment: Automated build and submit pipeline May 25, 2022
@philpax

This comment was marked as resolved.

@NotAdam

This comment was marked as resolved.

@philpax

This comment was marked as resolved.

@wolfcomp
Copy link

Just gonna quickly comment, is it possible to maybe extend the automated pipeline to be a sort of plug and compile GitHub CI/CD system instead of locked to a specific NuGet package is required. That would require a specific file output for the upload instead of a specific build system. This would possibly take a little longer to make, but would open up the gates a little for how many drawbacks there are going to be otherwise being locked to one specific NuGet package.

Making this comment because DalamudPackager errors on my system complaining about some files missing when it's not missing.

@lmcintyre
Copy link

Just gonna quickly comment, is it possible to maybe extend the automated pipeline to be a sort of plug and compile GitHub CI/CD system instead of locked to a specific NuGet package is required. That would require a specific file output for the upload instead of a specific build system. This would possibly take a little longer to make, but would open up the gates a little for how many drawbacks there are going to be otherwise being locked to one specific NuGet package.

Making this comment because DalamudPackager errors on my system complaining about some files missing when it's not missing.

I would assume this should be something to look into (more descriptive errors) in DalamudPackager rather than rethinking the process. There is definitely a file missing in your case - it's just extremely difficult to determine where, because the error message just says that it couldn't find a file.

@wolfcomp
Copy link

wolfcomp commented May 29, 2022

Just gonna quickly comment, is it possible to maybe extend the automated pipeline to be a sort of plug and compile GitHub CI/CD system instead of locked to a specific NuGet package is required. That would require a specific file output for the upload instead of a specific build system. This would possibly take a little longer to make, but would open up the gates a little for how many drawbacks there are going to be otherwise being locked to one specific NuGet package.
Making this comment because DalamudPackager errors on my system complaining about some files missing when it's not missing.

I would assume this should be something to look into (more descriptive errors) in DalamudPackager rather than rethinking the process. There is definitely a file missing in your case - it's just extremely difficult to determine where, because the error message just says that it couldn't find a file.

The error was in an outdated structure of SamplePlugin that was fixed with #12 that I followed to how I set up the project.

@philpax
Copy link
Contributor Author

philpax commented May 29, 2022

Yup, we sorted out Wolf's issue on the Discord (thanks for pointing it out!) Wolf's mentioned that they're happy with this now that their issue's been fixed 👍

As I mentioned on the Discord, my preference is to make the happy path trouble-free than to provide ways off the path. Happy to revisit once we get some more experience, though!

@philpax philpax merged commit 46a76ab into goatcorp:main May 31, 2022
This pull request was closed.
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.