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

Implement Github Actions Based Deployment #134

Closed
wants to merge 10 commits into from

Conversation

JayRovacsek
Copy link
Member

@JayRovacsek JayRovacsek commented Nov 3, 2024

This PR (un)fortunately also includes a raft of other changes than just the title. To get this deployment working, it was necessary to resolve a range of issues first, This PR should close:

Regarding #119; a range of types were changed on the types.ts file associated with events, when spot-checked upstream with meetup, it didn't seem that any fields that were previously marked as possibly null, ever actualised as a null value. Otherwise handing of null values for venue was handled for given I could evidence that as null via NCSG events as a test candidate (or location isn't easy to get Google maps to accept)

Notes on changes not directly tied to any of the above:

  • package.json license was made consistent with the license file
  • package.json authors was changes to an arbitrary reference to the general meetup community
  • gitignore and meetup files were lexicographically sorted to make it easier to identify if an entry exists already
  • dayjs was moved to luxon only as dayjs immediately threw errors when ts-node was added to the tsconfig.json configuration; a byproduct of getting the webpack build to propagate from either the index.html file or entries. I didn't dig into dayjs as I was extremely familiar with luxon as a pretty common datetime package for ts/js and was confident in it's suitabiltiy of use.
  • Any modified file was also linted via prettier at time of modification to ensure consistency; this may have caused changes to files that are simply lint-based but I attempted to keep this to a minimum.
  • This changeset will cause a conflict with fix#126: Added Rendering of Markdown for Event Descriptions. #128 and 🎨 Added github action to lint code on pull request #122 - I'm happy for those to be merged before this PR and I can resolve issues on this PR or I can work with the authors on resolving any issues with existing PRs. Alternatively I can amend this PR to include their changes and credit them as co-authors (though it will lack any of their signing or proof)

When assessing this PR, deployed artifacts exist to assist in review;

  • df91d36 was created intentionally with a non-main target to ensure generation of a pages instance that can be viewed here - this was changed to main in a later commit (edit: 0cbee4e) to ensure the merge candidate would work within the newwwie org. A showcase of the removal of commits being required for the deployment exists in this example where the source still describes an old update to the November Data Coders Group Meetup of 14 people but the deployed instance reflects correctly (at time of writing) 17 people.
  • Github pages configuration needs to be updated to utilise github actions deployment settings as opposed to branch deploy settings either during or post merge

There is some follow on issues we should consider if this PR is merged:

  • removal of historic event data from git history: more as an opportunity to reduce repository size

If resolving the conflicts in this PR with the primary branch, please consider accepting the removal of the dist folder as it is no longer required

edit; apologies if the raising of this PR interrupts anyone over the weekend 🙇

…nfig changes, migrate dayjs to luxon as a shortest path solve for type errors

refactor: lexicographically sort meetup json to make it easier to understand what communities exist in a reasonable way
refactor: change variable name to align with ts norms (camel)
note: this commit intentionally includes an incorrect push branch to validate the deployment prior to a merge request being created
@JayRovacsek JayRovacsek requested review from clbaita, neozenith and a team and removed request for neozenith and clbaita November 3, 2024 10:04
@neozenith
Copy link
Member

@JayRovacsek thanks for providing this PR. This is probably the most urgent PR to address to move to modern deployment methods for GHActions.

When I ran this though:

git clone https://github.com/JayRovacsek/newwwie.com/
cd newwwie.com
npm i
npm run dev

I got this:
Screenshot 2024-11-05 at 12 57 26 PM

Copy link
Member

@neozenith neozenith left a comment

Choose a reason for hiding this comment

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

npm install sass sass-loader@latest

Looks good. Just needs to update the Sass loader version

Also the quickstart instructions should now be:

npm ci
npm run fetchEvents
npm run build
npm run dev

@JayRovacsek
Copy link
Member Author

@JayRovacsek thanks for providing this PR. This is probably the most urgent PR to address to move to modern deployment methods for GHActions.

When I ran this though:

git clone https://github.com/JayRovacsek/newwwie.com/
cd newwwie.com
npm i
npm run dev

I got this: Screenshot 2024-11-05 at 12 57 26 PM

Oh - re; the above; I can create an issue for it, or look to solve for it still, but the deployed artefact won't show the above error - we do need to solve for it to ensure once saas kills v1 we're not impacted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants