-
-
Notifications
You must be signed in to change notification settings - Fork 649
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
Have PR go through the equivalent of "Deploy pants pex unstable" #7648
Comments
We've had many discussions about burning binaries on PRs in the past. I am generally against it. Twitter generally wants it. I want to confirm first this isn't an end run around those discussions. Last I recall Twitter had agreed documenting its requirements for it's build process publically would be a sane next step. IIUC that never happened. |
We've been able to unspend a lot of technical debt on this internal build/deployment process in the past year+, so we may be in a better position to answer this now. |
Also related to the build/deploy process internally: There are some related things that I would like to upstream (like performance testing), but @illicitonion and I were pretty unsure we could transfer our internal performance testing to upstream infrastructure due to the difficulty of getting reliable hardware specs (see tweet). I was thinking one alternative might be to look into how to account for hardware variance if there are known, good methods that other open source projects use to setup performance testing, especially in CI. Performance testing is semi-frequently an investigation that blocks our internal release, so (all imho) it could help to close the loop on fixing performance regressions in upstream pants in general if we could move any performance regression testing into upstream pants. But it's not clear how we'd want to do that / which use cases we would want to benchmark upstream. |
The pants release process produces pexes that are hosted on github, and the shards in master are intended to test that that process does not break (afaik). Currently, those shards can break post-merge to master. That is the key motivation here. The relevant issue in the most recent case was #7593. Yes, it is the case that relatively few consumers (if any, other than Twitter) use the pexes to consume pants, but continuing to head in the direction of dogfooding pex by releasing pants with it seems like a good direction. |
OK, assuming this is a good faith non end run then, it seems building the PEX and testing it runs is enough. No need to store it anywhere. |
The issue is that the wheel builder shards produce something that would need to be consumed by the pex builder shards. |
from op @wisechengyi:
Are the security requirements different than e.g. for the output of the bootstrap shards? It seems like there might be more artifacts we need to be publishing -- is that relevant, or is it some other reason? I cannot tell if there's context I'm missing here. |
I think John is probably right? The "Deploy pants pex unstable" shards can consume the artifacts from shard 1-4, but does not need to publish the final pex. |
Same as bootstrap I think, but turns out maybe we don't need to do that as described ^ |
Currently aim to work on this next week. I haven't looked the travis build for a while, so will circle back if things are not what I expected. |
Closing as being stale. Please feel free to reopen if this is still desired and anyone is interested in working on it. |
Problem
Currently PR does not and cannot trigger
Deploy pants pex unstable
as a merged change on master does (e.g. at the bottom of https://travis-ci.org/pantsbuild/pants/builds/520546720)This caused an issue where #7186 (Updating pex) broke
Deploy pants pex unstable
portion.Proposed Solution
To prevent future breakages, I'd like to propose that we run the equivalent of
Deploy pants pex unstable
in PR builds. Given the security concern, we can publish the wheels to a separate and untrusted place, different from where master would publish the wheels to.One issue might be how we can store the credentials for the separate s3 bin, but we can discuss how once folks are okay with the idea.
@jsirois @benjyw thoughts?
The text was updated successfully, but these errors were encountered: