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

Adding action to auto-update releases data on deployment #108

Closed
wants to merge 10 commits into from

Conversation

jonrohan
Copy link
Member

Addresses part of #102, It will still need to be manual for any news events.

I've created an action that runs the get-releases-data script before deploying to now. This makes the .json file ready when deployed.

Deployed here https://primer-style-auto-release-updates.now.sh/news the releases exist even though the data file was emptied in this PR.

cc @shawnbot @emplums @broccolini

@shawnbot
Copy link
Contributor

Dumb question: how do we test this locally?

@jonrohan
Copy link
Member Author

Dumb question: how do we test this locally?

the script should work locally without any setup. verify it updated releases.json file

@shawnbot
Copy link
Contributor

Okay, cool. Could we add a run-script for it in package.json then? E.g.

{
  "scripts": {
    "update-releases": "./.github/actions/releases/entrypoint.sh"
  }
}

@shawnbot
Copy link
Contributor

Okay now that I see how this works, I'm wondering: should we just do this in a prebuild script rather than running a separate action? 😬

@jonrohan
Copy link
Member Author

Okay now that I see how this works, I'm wondering: should we just do this in a prebuild script rather than running a separate action? 😬

I had it there at first but wasn't able to get it to work because of requirements 9c5de71 the build script failed, then the deploy script failed

@shawnbot
Copy link
Contributor

I had it there at first but wasn't able to get it to work because of requirements 9c5de71 the build script failed, then the deploy script failed

Which requirements? The slim image doesn't have bash?

@jonrohan
Copy link
Member Author

Which requirements? The slim image doesn't have bash?

jq mostly

@shawnbot
Copy link
Contributor

Okay, lemme see if I can fix that up. There's a (basically) drop-in replacement called fx that we can install via npm.

@jonrohan
Copy link
Member Author

The deploy script was failing I think because it couldn't see the script/ folder when it was trying to rebuild on deploy

@shawnbot
Copy link
Contributor

Ah, gotcha. So maybe we build the file in our actions and then make sure it's listed in files in now.json rather than making it part of the build script?

@broccolini broccolini added this to In progress in primer.style tracking Mar 17, 2019
@emplums
Copy link
Contributor

emplums commented May 24, 2019

@shawnbot are there any changes you'd still like to see on this or is this good to ship?

@shawnbot
Copy link
Contributor

@emplums Yeah, we've some conflicts to resolve first. I'm also still interested in knowing whether we can "just" move this to a prebuild script (using fx rather than jq) rather than having it happen in an Action, as that'll make it easier to test locally if/when we redesign the news page.

@shawnbot
Copy link
Contributor

Okay so I'm going to try to resolve the conflicts on this, and rebase if that doesn't work. If for some reason this PR doesn't jive with some more drastic changes scheduled in #152, then we may need to rethink the approach, e.g. getting releases with something like gatsby-source-github or gatsby-source-npm to render a static list at build time then (hopefully) have it hydrated with more timely data at runtime?

/cc @colebemis for ideas ⚡️

@shawnbot shawnbot mentioned this pull request Aug 5, 2019
@shawnbot shawnbot closed this in #164 Aug 7, 2019
primer.style tracking automation moved this from In progress to Done Aug 7, 2019
@shawnbot shawnbot deleted the auto-release-updates branch August 8, 2019 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants