-
Notifications
You must be signed in to change notification settings - Fork 4
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
Allow pulumictl get version
on bare repositories
#72
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Only nit, can we get a bit of -v
logging to help diagnose issues at runtime?
RefSpecs: []config.RefSpec{config.RefSpec( | ||
fmt.Sprintf("%[1]s:%[1]s", p.String()), | ||
)}, | ||
Tags: git.TagFollowing, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this fetch commits only or it fetches tags from the remote also if these commits are tagged on the remote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tags: git.TagFollowing
will fetch commits and any tag applied to them.
pkg/gitversion/gitversion.go
Outdated
if err != nil && !errors.Is(err, plumbing.ErrObjectNotFound) { | ||
return false, nil, fmt.Errorf("could not find local object: %w", err) | ||
} | ||
// We have found a local tag that is a parent, so return it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious is this behavior what pulumi build system needs? This would prefer a local tag. Suppose you forgot to do fetch tags. So origin has more recent tags than local. For example:
local: c1=v0.9 --> c2 --> c3=HEAD
origin: c1=v0.9 --> c2 --> c3=v1.0
Current algo will decide we're at 0.9-pre.something
If you had this though:
local: c1 --> c2 --> c3=HEAD
origin: c1=v0.9 --> c2 --> c3=v1.0
The system would decide we're at v1.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, I think the only way for pulumictl
to see a local and remote history as in your example (👇) is on a dev machine that has not fetched tags in a while.
local: c1 --> c2 --> c3=HEAD
origin: c1=v0.9 --> c2 --> c3=v1.0
I can change when we execute a remote search to only trigger when we are missing history, which would prevent this corner case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To step back why is it consulting both local and remote and tries to merge it? Can it just look in remote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hypothesis.
The operation you need in CI builds is this. Fetch me the bare --depth 1 commit X
from remote to get the code, and compute the version. To compute the version you only need to look in remote/origin.
The operation you need locally is this. I have a full non-bare repo and I need to compute version based on local tags ONLY. If I introduce tags I want it to see them. It's my job as dev to sync tags with remotes and so forth.
Can these two use cases perhaps be separate commands instead of coding up merging remote and local?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add a --remote={yes,no}
flag that defaults to no
. This will let us:
- Enable this behavior separate from a merge to master.
- Disable it by for local builds.
Well I sympathize with the desire to shave off 3m from this: https://github.com/pulumi/pulumi-aws/actions/runs/6166783348/job/16736792578 I however don't think this is a good idea to change pulumictl behavior in-place - at this point pulumictl is critical infrastructure and this can wreck things if there're mistakes rolled out. Are you confident in our testing strategy for pulumictl? I don't see tests added for new behavior, and specifying new behavior seems tricky. We already have users saying things like "I don't understand how pulumictl decided the version". |
Let's consider a few alternatives, LMK what do you think:
|
Also where is the 20 min figure coming from. An alternative to strongly consider also, is that we need to compute the version for the build only once and not recompute it in every build phase. The version can be redistributed to build phases as an artifact in GHA. Like in pulumi/pulumi. This is really important because absent that the builds are non-atomic on versions. If someone tags the repository while a build is in-flight, prerequisites phase will get one version while release phase will get another version. If pulumictl is invoked only once then this shaves off 3 min not 20 right? |
I think the real issue here is that our GH actions don't pin the pulumictl version. It should be possible to evolve a tool without breaking everything.
Yep, this is why I asked for some |
That's true. I'm getting some cold feet that this change is not going to break but I can work harder on review. Happy to rescind objection if y'all have confidence here. |
That's a good idea, but it will require a larger re-work of our CI and Makefiles, or adding an unexpected side channel to
|
04dcf39
to
15afdca
Compare
b4a8dc5
to
e4fb14f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually can we consider some alternatives here..
I have mentally a small complexity budget for the algorithm that needs to compute versions, and this codebase doesn't fit in this budget. It seems way too complex to me.
As it turns out we have prior art on the @pulumi/providers team of computing versions in what I consider a simple and accessible manner:
This does a lot right-ish:
- In release mode uses the tag.. $GITHUB_REF_NAME as is
- In dev mode computes an alpha version based on last release; last release polled from GH
- Atomically computes the version to keep it constant across the phases of the build
- It still uses pulumictl but only for triggering docs build?
I'm curious @danielrbradley it's not using pulumictl for version converters? What converts the versions to PyPI versions?
The only controversial thing above imo is computing the last release from GitHub metadata. This locks into linear flow so for example if main branch on AWS is on 5.x, I cannot open a v6 branch to publish alphas from and have dev builds on that branch realize they're v6. I wonder how azure-native does that.
I wonder if we considered just checking the baseline release version as .special-file
in-repo. Then it's trivial to read and has no constraints on branching. We just need to make sure that releasing updates its contents.
This looks like a possibile solution if it's a quick fix to address failures without having to restructure the workflows, but we'd need to weigh this with the added complexity as @t0yv0 mentioned. The azure-native approach still uses |
Yeah so folks I'm partially being somewhat reluctant to take the time to review this line-by-line to get confidence, it might be fine, but partially I'm just wearing the ops hat - every change, even a small change, has a non-zero risk. Given where our build system is today (open issue count) it's a bad time to take that risk on. This PR takes the risk on for the goal of speeding up builds, that's not a good tradeoff for me. If we push through with this we could reduce risk by:
All in all I prefer broadcasting the scheme from azure-native to bridged providers since it has much less surface for bugs to hide and also it's been in production use so that's something going for it. |
Previously,
pulumictl get version
required a full clone of the repository it was in. This was necessary so we could traverse history to find the nearest parent of the current commit.This PR enables us to perform the same algorithm on a repository with non-local commits, lazily fetching commits as needed.
This will allow us to avoid full clones of provider repositories in CI. For pulumi-aws, this will take ~20 minutes off of the time from new PR to release.