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

Dev -> Main: Prep for CI/CD roles refactor #138

Closed
wants to merge 1 commit into from
Closed

Dev -> Main: Prep for CI/CD roles refactor #138

wants to merge 1 commit into from

Conversation

thomasyu888
Copy link
Member

Push dev into main

@thomasyu888 thomasyu888 requested a review from a team as a code owner February 10, 2025 21:02
@thomasyu888 thomasyu888 changed the title Dev -> Main Dev -> Main: Prep for CI/CD roles refactor Feb 10, 2025
@philerooski
Copy link
Collaborator

Are we sure this is the best way to pull in these changes? Look at the network:

image

dev is just one commit ahead of main, yet this PR proposes merging 11 commits into main. Could we potentially rebase dev on the left-most commit in the network above, drop duplicated commits (giving preference to the commits in main), drop unnecessary merge commits, and force push this new, clean, and linear commit history to dev?

The only side effect I can see is that by giving preference to the commits in main, the drop-unused-resources branch will need to be fixed, which shouldn't be too difficult.

@jaymedina thoughts?

* Create proxy admin database role

* Grant proxy admin role ownership of *ALL_ADMIN roles

* Transfer ownership of current and future internamespace objects

* address PR comments

* bump version to avoid conflict

* grant execute managed task privilege to proxy admin
@philerooski
Copy link
Collaborator

philerooski commented Feb 12, 2025

It's not as pretty in the network diagram as I had hoped, but I can break down what's happening:

image
  • Those first seven blue dots/commits on main are duplicates of commits in dev. clean-dev uses the commits from main.

    • image
  • The next commits in dev and main after those seven duplicates are unique to dev or main, i.e., not duplicate commits. This gets a little complicated, because some of these commits contain brand new, unique changes, whereas others become redundant on account of the rebase: either because they are merge commits or commits which were made redundant by changes in previous commits. I've labeled them in the diagram below and I've also spelled out each commit after the diagram. Side note: The first unique commit, which happens to be from main, is the commit which clean-dev branches off from. Every subsequent unique commit – which could originate from either dev or main – has a new commit/hash in the clean-dev branch (new commits/hashes not shown in screenshot below).

    • clean_dev_unique_commits
    • Merge commits:
      • 4987fef, "Fix merge conflicts"
      • 1e10d25, "Merge branch 'main' into dev"
    • Unique commits:
      • 9f1b76f, "remove/revoke erroneous grants"
      • a645770, "Add Bishoy to Snowflake"
      • 5144fbf, "Fix syntax error from V1.6.0 admin script"
    • Redundant commits:
      • b215f57, "Update users.sql", which was made redundant by the unique commit a645770, "Add Bishoy to Snowflake"
      • 86117a6, "Update grants.sql" which was also made redundant by "Add Bishoy to Snowflake".
  • Finally, here is a screenshot of just those new commits/hashes in the clean-dev branch, starting with the unique commit "Add Bishoy to Snowflake" (new commit/hash 508a82e, as seen in screenshot below) and proceeding in chronological order of unique commits from there. The final commit is a new commit/hash of the HEAD of dev, 86a97c2, "Create proxy admin database role".

    • image

Go ahead and do a diff between dev and clean-dev. They are identical!

Since there are some redundant commits from main which we didn't use, we can force push clean-dev (minus its HEAD commit) to main. You can verify yourself that the commit 890f1ac, which is the commit before HEAD in clean-dev, is identical to main. Fortunately, there are no feature branches split from these redundant commits, so there's nothing to fix afterwards.

Then we can force push the HEAD of clean-dev to dev. The order doesn't actually matter, but fixing main (as the upstream branch) before dev (the downstream branch) feels right.

@danlu1
Copy link
Contributor

danlu1 commented Feb 13, 2025

@philerooski thanks for the clear explanation. I'm not very clear about when to use clean-dev. Just wanted to double check that I understand the situation. So basically, clean-dev is one commit ahead of main (Create proxy admin database role) and dev is two commit ahead of main (Create proxy admin database role + Tom's change). You would like to merge changes before "Create proxy admin database role" in clean-dev to main. Then, treat dev as the newest source of truth for changes need to be merged?

@philerooski
Copy link
Collaborator

philerooski commented Feb 13, 2025

@danlu1 The clean-dev branch is just for demonstration purposes. It's literally a "clean" version of the dev branch if we were to merge the commit histories of dev and main. The idea is that rather than including every commit in dev and main in some "merged" version of dev and main (imagine combining the main line in the diagram below with the dev line, even though we know many of those commits are duplicates/redundant) we create a cleaner commit history by picking all the unique / non-redundant commits and overwriting the commit history of both dev and main with this clean history.

image

This is what I did with clean-dev. I picked out the unique commits and discarded the redundant commits.

@danlu1
Copy link
Contributor

danlu1 commented Feb 13, 2025

@philerooski gotcha. Then I think this makes sense to me.

@jaymedina
Copy link
Contributor

@philerooski I love the breakdown with your diagrams, thanks very much for spending time on this!

Since there are some redundant commits from main which we didn't use,

Yes, in the network, that would be b215f57 and 86117a6, correct? Indeed these are redundant to 508a82e and can be removed.

we can force push clean-dev (minus its HEAD commit) to main

Right, so by doing this, we are matching up 5144fbf (from main) and 890f1ac (from clean-dev)

Just to be sure, once you've force-pushed the HEAD of clean-dev to dev, you will then rebase dev onto main so that these changes can take effect, correct? Just want to make sure this gets through the finish line so I can start validating it.

Other than that, all this makes sense to me and I agree with Dan, please move forward with this plan!

@philerooski
Copy link
Collaborator

@jaymedina

Just to be sure, once you've force-pushed the HEAD of clean-dev to dev, you will then rebase dev onto main so that these changes can take effect, correct? Just want to make sure this gets through the finish line so I can start validating it.

It's even simpler than a rebase. We are force pushing a.k.a overwriting and wholesale replacing the commit history of dev with the commit history of clean-dev. Afterwards, there's a few things we need to do so that our feature branches branch off from this new commit history:

  1. Your SNOW-147 branch currently branches off from 86a97c2 (the HEAD of dev), but it will need to be rebased upon 115f5ba (the HEAD of clean-dev, a.k.a. the HEAD of dev after we force push clean-dev to dev). This will be easy because the code base is identical between the HEAD of dev and clean-dev. You'll then need to force push to dev.
  2. My drop-unused-resources branch will need to be rebased since it branches from a (duplicate) commit in dev which we are throwing away. This will be pretty easy since the only changes in that branch are to add new files.
  3. All the other feature branches either branch off before cdcb9fd (the first commit which dev, clean-dev, and main have in common), or their changes have already been incorporated into clean-dev (post-validation-cleanup and fix-1.6.0 branches).

@jaymedina
Copy link
Contributor

OK that sounds good, let me know when it's time for me to do step 1.

But I think I need to reword my question because I'm not sure this answers it: At what point in the branch cleanup does 115f5ba end up in main?

@philerooski
Copy link
Collaborator

@jaymedina

At what point in the branch cleanup does 115f5ba end up in main?

Once I force push to main. I'll let you know once I've cleaned up.

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.

4 participants