Skip to content

Conversation

@sethrcamp
Copy link
Contributor

No description provided.

@sethrcamp sethrcamp requested a review from a team October 3, 2025 15:32
@claude
Copy link
Contributor

claude bot commented Oct 3, 2025

Claude finished @sethrcamp's task —— View job


Code Review Complete ✅

Todo List:

  • Run git diff to understand what changed
  • Analyze the changes for potential issues
  • Provide review feedback

Review Summary

Changes look good and are safe to merge. This PR correctly establishes new gitflow branch patterns for both CI/CD workflows and AWS Amplify deployments.

Changes Analysis

GitHub Actions Workflow (.github/workflows/github-actions-PR-validation.yml:9-10):

  • ✅ Added develop/**/* and release/**/* patterns to trigger PR validation
  • This ensures new gitflow branches are properly validated

Amplify Stage Stack (cdk/amplify-stage-stack.ts):

  • ✅ Added develop/* and feature/* to auto branch creation patterns (line 302)
  • ✅ Added develop/* to auto subdomain creation patterns (line 312)
  • ✅ Comment formatting improvements for better readability (lines 10-15)

Amplify Test Stack (cdk/amplify-test-stack.ts):

  • ✅ Added release/* to auto branch creation patterns (line 297)
  • ✅ Added release/* to auto subdomain creation patterns (line 307)
  • ✅ Same comment formatting improvements (lines 10-15)

No Issues Found

The changes are consistent with establishing a proper gitflow workflow:

  • develop/* branches → Stage environment (appropriate for integration testing)
  • release/* branches → Test environment (appropriate for release candidates)
  • feature/* branches → Stage environment (appropriate for feature development)

All patterns follow AWS Amplify conventions and the changes are well-structured.


💡 Claude Code Capabilities

I can help with additional commands! Try these:

  • Answer Questions: @claude explain how the caching system works
  • Implement Code Changes: @claude add error handling to the user login method
  • Debug Issues: @claude help debug why the search is slow
  • Analyze Architecture: @claude review the database schema changes
  • Perform another review: @claude Please review my changes

Just comment @claude followed by your request!

Copy link
Member

@mluypaert mluypaert left a comment

Choose a reason for hiding this comment

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

@sethrcamp I have a few question on these changes:

  • What's the logic behind having all release/**/* branches use the test environment configurations? Shouldn't release prep branches use stage data etc. instead (as that's where new releases usually originate from)?
  • What's the intended use of the develop/* and feature/* branches (as opposed to SCRUM-* and KANBAN-* branches)?
  • What's the reason why feature/* branches are only added for automatic amplify branch creation but not added to the alliancegenome.org domain? Are these intended for internal testing only?

@sethrcamp
Copy link
Contributor Author

@sethrcamp I have a few question on these changes:

  • What's the logic behind having all release/**/* branches use the test environment configurations? Shouldn't release prep branches use stage data etc. instead (as that's where new releases usually originate from)?
  • What's the intended use of the develop/* and feature/* branches (as opposed to SCRUM-* and KANBAN-* branches)?
  • What's the reason why feature/* branches are only added for automatic amplify branch creation but not added to the alliancegenome.org domain? Are these intended for internal testing only?

In general, the ui working group is electing to try out git flow for our branching strategy, mainly to avoid these cart and horse situations like we have for the last release. We don't expect the A-team to use git flow (although these changes would work if they did decide to also use it in the future).

In response to your specific questions:

  1. Release branches are intended as a last step before pushing to production/main. They function very similarly to how we utilize the current test branch. The main difference is that a new release branch is made each time you want to push a batch of code out, as opposed to having one branch that always sticks around. This allows us to merge specific features onto a release branch and push code to production without having to wait for the next official site release. It also allows for development to continue for future work if we are nearing a site release, since there can be more than one release branch at a time.
  2. Develop branches work exactly as the stage branch works for the A-team. I.e. they are in active development and should always be in a state that can be deployed (even if it has unfinished features, or content not ready for the public). Feature branches are specific to individual tickets, much like how we use ticket numbers to name our branches as we work on new features. Using "feature/KANBAN-XXX" instead of just "KANBAN-XXX" is one of git flow's semantics. Additionally, if there is a ticket that needs to be pushed to production in isolation to fix a breaking change, using the prefixes allows you to also name branches like "hotfix/KANBAN-XXX" or "release/KANBAN-XXX". Essentially, the prefix denotes what environment should be used to deploy.
  3. Yes, feature branches are always internal, so I figured the auto-generated version should be good enough (though I don't have a super strong opinion on it). Also, I noticed that those patterns for auto subdomains are case specific, so we haven't actually been using that feature since "SCRUM-XXX" won't trigger the pattern of "scrum-". I allowed for "develop/" to create subdomains since these essentially work like stage does now. The UI group is going to utilize the "develop/ui" branch, and that will get a subdomain of develop-ui.alliancegenome.org

@mluypaert
Copy link
Member

In general, the ui working group is electing to try out git flow for our branching strategy, mainly to avoid these cart and horse situations like we have for the last release. We don't expect the A-team to use git flow (although these changes would work if they did decide to also use it in the future).

Okay, I think the general idea is good but you'll need to ensure the A-team (and anyone else possibly working on this repository) is aware of these changes in branching strategy to ensure everyone adheres to them (in this repository), as branching strategies always need to be applied consistently accross the entire repository to work as intended. I think it would be good to raise this plan on a dev coordination call (today or as soon as possible).

  1. Release branches are intended as a last step before pushing to production/main. They function very similarly to how we utilize the current test branch. The main difference is that a new release branch is made each time you want to push a batch of code out, as opposed to having one branch that always sticks around. This allows us to merge specific features onto a release branch and push code to production without having to wait for the next official site release. It also allows for development to continue for future work if we are nearing a site release, since there can be more than one release branch at a time.

Be careful with the naming here, as this every quickly can get confusing. In the past we've always used release-vx.y.z branches to prepare public releases including new data and code originating from the stage branch (so same intent as what you've described here). However, as release branches are always intented to be merged back in main, you should not have multiple live release branches, as the merge of any one of them into main invalidates all others, which then immediately need to merge the newly introduced changes (from main) back to represent complete code for accurate pre-release testing.
Therefor, when following gitflow logic, all development work for future releases (for any release other than the next one being tested on a release[-/].+ branch) should go in a stable development branch such as develop (more on that further down), and a release branch should only be branched from that stable development branch when intending to release (not during active feature development).

  1. ... I allowed for "develop/" to create subdomains since these essentially work like stage does now. The UI group is going to utilize the "develop/ui" branch, and that will get a subdomain of develop-ui.alliancegenome.org

I've you're planning on using a develop/ui branch as the UI WG's stable development branch, then this is essentially the same as renaming the current test branch (and test.alliancegenome.org) to develop/ui? In which case, the test branch (and it's corresponding amplify configs) would need obsoletion/cleanup and I don't see what you're really gaining (other than maybe improving some branch naming)?
Additionally, as the stage branch should be considered the stable development branch for all things involving data changes, having multiple stable development branches is exactly what's causing the cart and horse situations you're trying to avoid. As the stage branch needs to remain the stable development branch for all things involving data changes (no matter what you name it), I would suggest not to create any (other) develop/* branches, and instead only work with feature/* branches that are intended to merge into stage, hotfix/* branches that are intended to merge into main, and releasefix/* branches that are intended to merge into any release/* branches (which temselves are intended to merge into main). main, stage, and release/* branches can have their representation on the alliancegenome.org domain (as www, stage and release-* respectively), hotfix/*, releasefix/* and feature/* branches are probably all fine to have an internal amplify URL only.

One further question, I've only just noticed this PR is opened to merge in the stage branch. I assume you're aware that means these changes (to amplify) will only become active on the next public site release (once merged into the main branch), I'm not sure when that's planned?

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.

3 participants