acmucsd / membership-portal

REST API for the UC San Diego ACM chapter's membership portal.
https://members.acmucsd.com
Mozilla Public License 2.0
17 stars 5 forks source link

restructure repo to publish types properly to npm registry #135

Open sumeet-bansal opened 3 years ago

sumeet-bansal commented 3 years ago

Published types should only have required dependencies and we should limit the manual work required to version properly.

I imagine this would take the form of a multi-package repo managed by lerna or some similar tool, and possibly some semantic release tool like semantic-release kicked off by our CI pipeline.

sumeet-bansal commented 3 years ago

@shravanhariharan2 gonna tackle this next since Sage Team and Dimensions are going to be using our API. Thoughts?

Our goals:

I looked into some existing tools but they don't play well with lerna and weirdly rely on commit message structures which seem pretty liable to break since it's hard to enforce that on PR titles. But the general idea for them's that you commit to master and format the commit message in a parseable way, then it'll bump the version, commit again to master, and publish to npm and GitHub for you. The way I outlined below requires slightly more legwork upfront but is a little cleaner I think because it lets us update versions in their respective PRs instead and still publish npm and GitHub releases without enforcing any rules on commit messages.

For publishing:

Long-term it'd be nice to automate versioning with a GitHub bot or something (it comments on your PR with a checklist like ["improvement";"patch";...], you check a box, and it commits your version bump), and auto-generate docs from code like TypeDoc but for Postman, but those aren't important right now.

shravanhariharan2 commented 3 years ago

Couple comments/questions:

general idea for them's that you commit to master and format the commit message in a parseable way, then it'll bump the version, commit again to master, and publish to npm and GitHub for you

Are two commits necessary? And how are they supposed to be formatted (and is that taken care of by the CI before merge)?

restructure the repo as 2 separate packages (the API spec and the API implementation) using lerna, maybe a third package for tests

Thoughts on also adding seeds/fakes as a package? Not sure how this would work since a lot of it requires implementation functionality, but a developer could work directly with portal state programatically. Not sure how useful this might be but let me know your thoughts.

CI check (manual approval) where developers confirm they documented their API changes if needed

Not sure what you mean by manual approval for check - does this appear in CircleCI as a task with no actual job to run?

Also, this documentation refers to our Postman docs right? Right now that's only accessible from our ACM development account, dev@acmucsd.org. I temporarily moved it there since free Postman only allows 25 shared requests (it prevented me from adding/editing any requests after that), so should we move to a paid plan ($12-15 per user per month)? Alternatively we could have everyone working on one Postman account to make it free free but thats a huge hassle to manage credentials

Also, how does it work of two people have different PR's that both need to bump versions? You mentioned that we can update versions within the PR, but this might cause conflict with other PR's right? Or is that bump only on merging, where the other PR can merge those changes & bumps and bump their own after that? But then wouldn't they be blocked by the other PR merging first before they version bump?

Overall, it seems pretty clean but a little complicated of a change just to add versioning for types/specs, but looks like this will help down the road w/ documentation + API wrappers and it doesn't look like there's any cleaner & automated alternatives.

sumeet-bansal commented 3 years ago
  1. I think some can amend commits but git history should be immutable, so I'm not considering that an option, although I guess it's very unlikely to cause conflicts since the amend happens within a minute of merging. Since the commit messages are the PR titles, I don't think that's something CI can handle?
  2. Seeds and fakes would go in the testing package. Packages can use each other, so implementation would use types and tests would use implementation and types.
  3. Manual approval means it's a CircleCI task with no job, so in the list of PR checks you click on the approval checks and click yes/no (or something like that). See here for details on CircleCI manual approval.
  4. Yes, our Postman docs. Postman isn't worth paying $12-15/user/month, and we already have single accounts for Heroku, Netlify, etc.
  5. If one PR merges a version bump, it should cause a merge conflict in another PR with its own version bump since they're both making changes to the same lines.
  6. Yeah it's a good bit of work but it's not really for versioning so much as it is for publishing our API specs for clients like UI, Sage, and Dimensions to use. Since I'm sure other teams are going to be building off the portal in the future, we should do publishing right, and versioning is a part of that.
sumeet-bansal commented 3 years ago

@shravanhariharan2 any other thoughts on this?

shravanhariharan2 commented 3 years ago
  1. So do PR titles need to be formatted a specific way? Like how would those messages be formatted / is that something we need to standardize in PR titles?

Rest sounds good to me

sumeet-bansal commented 3 years ago

PR titles don't need to be specifically formatted, but I'll switch to uppercase imperative to match your PRs since that's more common.

shravanhariharan2 commented 3 years ago

ok, and are we trying to match commit formats too? Like imperative vs. past, upper vs. lower, or does that not matter since the PR title is what gets commited to master anyway?

sumeet-bansal commented 3 years ago

Commits in PR branches don't matter.

shravanhariharan2 commented 3 years ago

@sumeet-bansal @michl1001 there's been some interest in getting this set up within the next few weeks for ease of development in frontend for merch store (plus other QoL things for frontend with the API wrapper). Since we aren't necessarily working on any features currently, I'm looking to dig deeper into what we want out of a restructure like this.

Some questions to consider / discuss:

packages/
    types/
    api/
    seeds/
    impl/(?)
    ...

initially might work but I'm not sure what's the best way to split implementation from spec in a package sense.

I think we should also definitely consider writing the API wrapper now as well since it makes testing a lot easier. As I've been working more on testing I'm realizing how much easier it'd be to have actual API calls rather than directly using the controller methods, since things like validating multer storage and other types of validation are either difficult or are just not possible to do with the current setup.

sumeet-bansal commented 3 years ago

I think our first goal's to publish types, and the API wrapper can be done afterwards.

What can't we test with our current setup?

shravanhariharan2 commented 3 years ago

One thing I'm unable to test now is if an error is thrown if a file passed in is of too large of a size to the upload controllers (e.g. event cover upload). From what it looks like a lot of the processing/validation happens under the hood of @UploadedFile and isn't testable with class-validator, so I don't think we can test the "too large files" assertion.

If that's a minor thing (which is what it seems like) then I can just continue writing some more tests to cover the essential things (events and attendance) before moving forward.