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

Git Repository Overhaul #155

Merged

Conversation

JustinFirsching
Copy link
Contributor

@JustinFirsching JustinFirsching commented Nov 4, 2021

Update the pull function so that tag-provided mirrors do not fetch all
tags, instead later fetching (externally) only the tag they need.

Implement tag fetching function to retrieve only the desired tag when
creating a tag-provided repo mirror.

Checkout the tag into a detatched HEAD state at the end of the create
stage of tag-provided repository mirrors.

Implement ref removal and addition functions used during package
creation and mirror push to ensure that refs that aren't wanted on the
mirror won't be pushed.

Update the refspecs used in the push to push detatched HEAD, branches,
online remote, and tags to the offline mirror. Note that if we later
checkout a branch from the remote and do not clean up the remote ref it
will lead to a duplicate ref name and the push will fail on one of the
refs (likely the online one since it is later in the refspec slice).

Fixes #154

feat: Allow for repos to be provided without a tag to mirror all branches/tags
feat: Make tag-provided repository mirrors use the tag as master
fix: Prevent tag-provided repo mirrors from storing extra refs

@RothAndrew
Copy link
Contributor

I'll let @jeff-mccoy be the final say here but I don't think this is something we want to do. We want the declarative state to be a specific git reference. Its an outstanding bug that zarf package create actually clones more than is intended.

@JustinFirsching
Copy link
Contributor Author

Responded in the issue as well, but to make sure it is seen here too:

Would it be fair to say that if there is no tag provided everything is brought over, but if a tag is provided only that tag is brought over? If so, I could likely get the rest of this implemented this week.

@jeff-mccoy
Copy link
Contributor

I commented on the issue, open to discussing this as an opt-in, but the reason for tag-based tracking is we will be using that to do diffs in the future without having to fetch each remote asset and see if it changed. In git tags can’t be moved, they can only be removed/ added (ie forced) but that’s not the standard convention so it’s a good way to track changes quickly. Long term idea would be a new command to compare a current zarf.yaml to an older one to see what resources have changed and only include those.

@jeff-mccoy
Copy link
Contributor

/test all

@JustinFirsching JustinFirsching changed the title feat(deploy): Make git push all branches Git Repository Overhaul Nov 7, 2021
@jeff-mccoy
Copy link
Contributor

/test all

@jeff-mccoy
Copy link
Contributor

@YrrepNoj please review for code flow. @RothAndrew please review for overall user flow and how this impacts current examples such as the Big Bang example.

@jeff-mccoy
Copy link
Contributor

I’ll do a final review after both of you.

@RothAndrew
Copy link
Contributor

@JustinFirsching pls pull in latest master changes

@JustinFirsching
Copy link
Contributor Author

@RothAndrew Sorry about that. Rebased and pushed.

@RothAndrew
Copy link
Contributor

No worries, just wanted to make sure the stuff added in 0.13 got included, namely the change to the private registry to require auth.

@RothAndrew
Copy link
Contributor

/test all

YrrepNoj
YrrepNoj previously approved these changes Nov 8, 2021
Copy link
Contributor

@RothAndrew RothAndrew left a comment

Choose a reason for hiding this comment

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

  • When deploying the gitops-service-data example based on the zarf.yaml I expect to see 3 repos, but I only see 1.
    image

  • Recommend having the gitops-service-data example pull in a helm chart that gets deployed. I see the Twistlock chart getting pulled in in the zarf.yaml but I don't really have any good way to verify that it is there

  • examples/gitops-data/README.md should be updated to talk about the different behaviors being demoed (single tag & full repo)

  • IMO we should add an E2E test for the gitops-service-data example but I won't hold up the PR for it if everyone else just wants to keep pushing forward

@JustinFirsching
Copy link
Contributor Author

@RothAndrew

  • Is it possible you are using an old copy of the Zarf package? I am getting all three repositories as shown in the screenshot attached. Running tar -I unzstd -tf zarf-package-gitops-service-data.tar.zst | grep 'mirror__[^/]*/.git/$' should list all of the repos in the package, and I expect the tar you have will only have the one repository.
    image
  • Would I just need to deploy the chart using a Kubernetes manifest? If so, I think that would look like joining the current (master) version of examples/gitops-data with examples/single-big-bang-package, which I can do, but want to verify before creating a duplicate code chunk.
  • As for the README.md, I won't have a chance to get around to that tonight, but can definitely get that updated to explain the new features.
  • Assuming all that needs to be done for the E2E test is updating the e2e.sh script to include a few calls to the Gitea API confirming that a gitops-data deployment pushed the repositories as expected, I can toss that in with the README.md updates.

@RothAndrew
Copy link
Contributor

Its possible. Ill check again im the morning

@RothAndrew
Copy link
Contributor

e2e.sh is the old way that we are moving away from. The new way is golang tests with the Terratest library, in the /test directory. If you feel comfortable taking a stab at it thats great. If not that's okay too, we can make an issue to follow up with a new test

@RothAndrew
Copy link
Contributor

Is it possible you are using an old copy of the Zarf package? I am getting all three repositories as shown in the screenshot attached. Running tar -I unzstd -tf zarf-package-gitops-service-data.tar.zst | grep 'mirror__[^/]*/.git/$' should list all of the repos in the package, and I expect the tar you have will only have the one repository.

Yep, I had master checked out rather than the branch you're working in. In your branch I see all 3 repos, and everything looks good inside them. The Zarf repo only has master (at the point in time when 0.12.0 was cut) and the 0.12.0 tag, and podinfo and twistlock have everything 👍

Would I just need to deploy the chart using a Kubernetes manifest? If so, I think that would look like joining the current (master) version of examples/gitops-data with examples/single-big-bang-package, which I can do, but want to verify before creating a duplicate code chunk.

IMO I'd actually just take out the chart from the gitops-data example. That functionality is already demoed in a couple of other places, including the single-big-bang-package example.

As for the README.md, I won't have a chance to get around to that tonight, but can definitely get that updated to explain the new features.

👍

Assuming all that needs to be done for the E2E test is updating the e2e.sh script to include a few calls to the Gitea API confirming that a gitops-data deployment pushed the repositories as expected, I can toss that in with the README.md updates.

See above comment from last night

@jeff-mccoy
Copy link
Contributor

/test all

@JustinFirsching
Copy link
Contributor Author

Updated the README as a new commit to make it clear that nothing else was changed. Let me know what still needs to be changed and I can rebase from there.

I haven't looked into the E2E tests much, but I don't have an AWS account, so if I understand correctly that may rule me out entirely.

@RothAndrew
Copy link
Contributor

Updated the README as a new commit to make it clear that nothing else was changed. Let me know what still needs to be changed and I can rebase from there.

I haven't looked into the E2E tests much, but I don't have an AWS account, so if I understand correctly that may rule me out entirely.

The tests would run in our account, they'd just need to be written and wired up, but I don't necessarily expect everyone to do that, at least until we write a guide on how to do it.

This can be merged without changes to the E2E test suite with an issue to have one of the core devs to do it.

@jeff-mccoy
Copy link
Contributor

/test all

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

Overall this looks good, some minor comments on some conditionals that are a little unclear (I patched one of them already) and look to @RothAndrew's final review as well. Think we're very close. Thanks for the work on this, it will make git operations much more robust.

cli/internal/git/utils.go Outdated Show resolved Hide resolved
@JustinFirsching
Copy link
Contributor Author

Thanks! I will get the requested changes up as soon as possible.

Looking forward to the final review. I assume it would be easiest to review as separate commits since @RothAndrew has been tracking along the way, but if that is not the case, let me know and I can force push with the commits squashed.

This was a much more interesting and complex issue than I expected it to be when I first filed the issue and I'm glad to have been able to contribute so much of the solution.

@RothAndrew
Copy link
Contributor

You can go ahead and squash if you want. Im off tomorrow but I'll take a look if I have some down time. I just had a couple of nitpicks on the documentation that may have been resolved now. @jeff-mccoy if you feel good about it thats good enough for me

@jeff-mccoy
Copy link
Contributor

I’d say press with the squash and run a “/test all” @JustinFirsching

@jeff-mccoy
Copy link
Contributor

/test all

Update the pull function so that tag-provided mirrors do not fetch all
tags, instead later fetching (externally) only the tag they need.

Implement tag fetching function to retrieve only the desired tag when
creating a tag-provided repo mirror.

Checkout the tag into a detatched HEAD state at the end of the create
stage of tag-provided repository mirrors.

Implement ref removal and addition functions used during package
creation and mirror push to ensure that refs that aren't wanted on the
mirror won't be pushed.

Update the refspecs used in the push to push detatched HEAD, branches,
online remote, and tags to the offline mirror. Note that if we later
checkout a branch from the remote and do not clean up the remote ref it
will lead to a duplicate ref name and the push will fail on one of the
refs (likely the online one since it is later in the refspec slice).

Fixes zarf-dev#154

feat: Allow for repos to be provided without a tag to mirror all branches/tags
feat: Make tag-provided repository mirrors use the tag as master
fix: Prevent tag-provided repo mirrors from storing extra refs
fix: tag-provided clones use trunk branch name
docs: Update gitops-data Example README
@jeff-mccoy
Copy link
Contributor

@JustinFirsching once the review comments I made a couple days ago are addressed I think we can proceed to squash/merge this PR.

@JustinFirsching
Copy link
Contributor Author

Sorry for the delay on this! Made the changes and got sidetracked before I had a chance to rebase and push. I don't think I have permissions to run tests, so someone else may need to kick those off for me, but I think this is what we are looking for. Happy to resolve any remaining issues with the changes and/or documentation associated with it!

@JustinFirsching
Copy link
Contributor Author

/test all

1 similar comment
@RothAndrew
Copy link
Contributor

/test all

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

everything looks good, thanks for all the work on this!

examples/gitops-data/README.md Show resolved Hide resolved
@jeff-mccoy jeff-mccoy merged commit 180fcef into zarf-dev:master Nov 14, 2021
@JustinFirsching
Copy link
Contributor Author

Glad to contribute! Thank you all for the feedback and being so responsive on the PR and issue thread!

jeff-mccoy pushed a commit that referenced this pull request Feb 8, 2022
Update the pull function so that tag-provided mirrors do not fetch all
tags, instead later fetching (externally) only the tag they need.

Implement tag fetching function to retrieve only the desired tag when
creating a tag-provided repo mirror.

Checkout the tag into a detatched HEAD state at the end of the create
stage of tag-provided repository mirrors.

Implement ref removal and addition functions used during package
creation and mirror push to ensure that refs that aren't wanted on the
mirror won't be pushed.

Update the refspecs used in the push to push detatched HEAD, branches,
online remote, and tags to the offline mirror. Note that if we later
checkout a branch from the remote and do not clean up the remote ref it
will lead to a duplicate ref name and the push will fail on one of the
refs (likely the online one since it is later in the refspec slice).

Fixes #154

feat: Allow for repos to be provided without a tag to mirror all branches/tags
feat: Make tag-provided repository mirrors use the tag as master
fix: Prevent tag-provided repo mirrors from storing extra refs
fix: tag-provided clones use trunk branch name
docs: Update gitops-data Example README
Signed-off-by: Jeff McCoy <code@jeffm.us>
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
Update the pull function so that tag-provided mirrors do not fetch all
tags, instead later fetching (externally) only the tag they need.

Implement tag fetching function to retrieve only the desired tag when
creating a tag-provided repo mirror.

Checkout the tag into a detatched HEAD state at the end of the create
stage of tag-provided repository mirrors.

Implement ref removal and addition functions used during package
creation and mirror push to ensure that refs that aren't wanted on the
mirror won't be pushed.

Update the refspecs used in the push to push detatched HEAD, branches,
online remote, and tags to the offline mirror. Note that if we later
checkout a branch from the remote and do not clean up the remote ref it
will lead to a duplicate ref name and the push will fail on one of the
refs (likely the online one since it is later in the refspec slice).

Fixes #154

feat: Allow for repos to be provided without a tag to mirror all branches/tags
feat: Make tag-provided repository mirrors use the tag as master
fix: Prevent tag-provided repo mirrors from storing extra refs
fix: tag-provided clones use trunk branch name
docs: Update gitops-data Example README
Signed-off-by: Jeff McCoy <code@jeffm.us>
@phillebaba phillebaba mentioned this pull request Jul 31, 2024
2 tasks
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.

Make Zarf Deployment Push All Git Repository Branches
4 participants