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

Prototype build artifact #98

Merged
merged 7 commits into from
Mar 26, 2020
Merged

Prototype build artifact #98

merged 7 commits into from
Mar 26, 2020

Conversation

namoscato
Copy link
Collaborator

This doesn't buy us much but also doesn't hurt in my opinion. I mostly wanted to prototype artifact functionality mentioned in #96 (comment). If we like this approach, we might consider uploading an artifact when linting fails to minimize some of the verbose stdout.

Specifically, this:

  • Separates GitHub Action npm commands into separate steps
  • Sets GAPI_MAX_PARALLEL: 3 in build / generate job
  • Uploads generated types as an artifact in build / generate before deploying*

*I tested this on a forked repository; here's what it looks like:

Screen Shot 2020-03-21 at 10 46 39 AM

@Maxim-Mazurok
Copy link
Owner

My problem with leaving JSON in the types is that we don't want to publish this JSON to the DT. So, I would argue for leaving .tmp directory. Then, we can upload both .tmp and types to build artifacts, and publish only types directory.

.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
namoscato and others added 3 commits March 22, 2020 12:22
…generator into build-artifact

* 'master' of github.com:Maxim-Mazurok/google-api-typings-generator:
  #96 (comment)
@Maxim-Mazurok
Copy link
Owner

Whould be great to have it finalized, because because my approach of console.logging turned out to be failing for large files. console.log has a limit, so I will have to switch to manually writing to stdout. Instead of applying this fix, I would prefer merging this PR :)

@namoscato
Copy link
Collaborator Author

Sorry, I'll try to circle back to this later today or tomorrow!

Copy link
Collaborator Author

@namoscato namoscato left a comment

Choose a reason for hiding this comment

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

@Maxim-Mazurok, I think I came up with a solid solution that hopefully works for you as well!

This still uploads the full types artifact on success, but to address your lint failure scenario, I ended up leveraging a workflow command to upload the specific types associated with the service that failed.

.github/workflows/build.yml Show resolved Hide resolved
bin/lint.ts Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
if: failure()
with:
name: ${{ env.FAILED_TYPE }}
path: types/${{ env.FAILED_TYPE }}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Learning more about expression syntax, there is probably an option to minimize some of this duplication, consolidating workflows in favor of conditional logic, but this is duplicated for now.

Copy link
Owner

@Maxim-Mazurok Maxim-Mazurok left a comment

Choose a reason for hiding this comment

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

Very nicely done 👍 Liked the `::set-env`` approach, thank you!

And yes, it would be nice to figure out how to eliminate duplication from the workflows in the future, create a new issue for this, please.

@Maxim-Mazurok Maxim-Mazurok merged commit eeee556 into master Mar 26, 2020
@namoscato namoscato deleted the build-artifact branch March 26, 2020 12:02
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.

None yet

2 participants