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

1-1531: create db table for cr schedules #5148

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

thomasheartman
Copy link
Contributor

This PR adds a db table for CR schedules. The table has two columns:

  1. change_request :: This acts as both a foreign key and as the primary key for this table.
  2. scheduled_at :: When the change is scheduled to be applied.

We could use a separate ID column for these rows and put a unique constraint on the change_request FK, but I don't think that adds any more value. However, I'm happy to hear other thoughts around it.

@vercel
Copy link

vercel bot commented Oct 25, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 25, 2023 0:24am
1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
unleash-docs ⬜️ Ignored (Inspect) Visit Preview Oct 25, 2023 0:24am

Copy link
Contributor

@kwasniew kwasniew left a comment

Choose a reason for hiding this comment

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

I like you approach in this PR

@thomasheartman thomasheartman merged commit a5d304c into main Oct 25, 2023
16 checks passed
@thomasheartman thomasheartman deleted the 1-1531-update-db-to-add-scheduled-changes branch October 25, 2023 12:36
@gastonfournier
Copy link
Contributor

Nitpicking: please remember to use conventional commits i.e. feat(1-1531) instead of 1-1531

@thomasheartman
Copy link
Contributor Author

@gastonfournier Good point! I forgot because I used a linear branch name for this instead. But help me out on this one: would it be a feat or a chore? Because this PR itself doesn't actually introduce a feature, so it shouldn't cause a minor version bump. It'd be a patch bump. Maybe a "refactor"? How do we deal with it when a feat is broken up into multiple steps where only the last step actually makes it public?

@gastonfournier
Copy link
Contributor

I used a linear branch name for this instead

The branch name shouldn't be a problem, it's just the title when you hit squash and merge. So it's easy to forget, and I do as well. We talked about enforcing this but we thought about a best-effort approach first.

How do we deal with it when a feat is broken up into multiple steps where only the last step actually makes it public?

What I do (not necessarily correct) is to do feat(myFeature): commit is how I aggregate small chunks of myFeature. Using chore instead of feat can work as well.

@thomasheartman
Copy link
Contributor Author

The branch name shouldn't be a problem, it's just the title when you hit squash and merge. So it's easy to forget, and I do as well. We talked about enforcing this but we thought about a best-effort approach first.

No, of course, it's just that the commit title is taken from the PR title, which is taken from the first commit I make for that PR (if you create the PR when there is only one commit). But I agree, this is just an oversight on my end, so thanks for picking up on it and reminding me.

What I do (not necessarily correct) is to do feat(myFeature): commit is how I aggregate small chunks of myFeature. Using chore instead of feat can work as well.

Yeah, that works, but doesn't matter per commit when it gets squashed down. But the interesting question here is: when you merge part of a feature, but not enough to make that feature active yet (such as just a DB migration), should the merge commit be a "feat"? Because on its own it doesn't do anything, so it shouldn't bump the minor version.

@gastonfournier
Copy link
Contributor

Yeah, that works, but doesn't matter per commit when it gets squashed down. But the interesting question here is: when you merge part of a feature, but not enough to make that feature active yet (such as just a DB migration), should the merge commit be a "feat"? Because on its own it doesn't do anything, so it shouldn't bump the minor version.

I mean on the squash I do feat(myFeature), when I meant "commit" I meant the commit on the main branch. And we never bump minor versions based on feat commits, we aggregate many every month and I believe the rationale should be: if we have at least 1 new feat we should bump the minor version.

What do we consider a feature? I think that's up to each engineer, but we should consider whether or not it deserves a minor version. Everything that can be a patch version should not use feat: according to conventional commits. But I'm no expert on this, so we should probably socialize this when we have the chance

@thomasheartman
Copy link
Contributor Author

And we never bump minor versions based on feat commits, we aggregate many every month and I believe the rationale should be: if we have at least 1 new feat we should bump the minor version.

No, I agree, but consider this: if you mark anything that's part of a feat (even if it's not the whole feat), and a release only has "partial" feats - meaning there's no real need to bump the minor because nothing has changed for the user - what should we do? I'd say that's a patch bump. But maybe it's a moot point because that's not likely to happen? But we also do patch releases regularly 🤔

In short, I don't mind, but I wouldn't call this a "feat" because it doesn't do anything for the user. I'd lean towards "chore" for this, but 🤷🏼

@gastonfournier
Copy link
Contributor

And we never bump minor versions based on feat commits, we aggregate many every month and I believe the rationale should be: if we have at least 1 new feat we should bump the minor version.

No, I agree, but consider this: if you mark anything that's part of a feat (even if it's not the whole feat), and a release only has "partial" feats - meaning there's no real need to bump the minor because nothing has changed for the user - what should we do? I'd say that's a patch bump. But maybe it's a moot point because that's not likely to happen? But we also do patch releases regularly 🤔

It doesn't have to be customer-facing to be a feature, but yeah if the feature is still partial it shouldn't require a minor bump, so on that train of thought we should only feat: when finishing the feature

In short, I don't mind, but I wouldn't call this a "feat" because it doesn't do anything for the user. I'd lean towards "chore" for this, but 🤷🏼

Agreed!

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

Successfully merging this pull request may close these issues.

None yet

3 participants