-
Notifications
You must be signed in to change notification settings - Fork 56
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
Drop official
concept, replace with hardcoded list of well-known protected Jenkins instances
#739
Conversation
utils.groovy
Outdated
jq -r '.metadata.annotations["jenkins.io/default-channel"]' | ||
""") | ||
} | ||
|
||
// Emits Slack message if set up, otherwise does nothing. | ||
def trySlackSend(params) { |
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.
Personally, I think the Slack integration is a good example of how Jenkins pipelines can grow in an ad-hoc/unbounded way. IMO, a better model is for things like chat notifications to poll (with opt-in event triggers) to send a message as part of a separate process (container). This is how e.g. the Prow job alerts work as far as I know.
utils.groovy
Outdated
// S3 bucket. | ||
def protected_buckets = PROTECTED_JENKINSES.collectEntries{jenkins_url, v -> | ||
def (bucket_prefix, allow_hotfixes) = v | ||
[splitBucketKey(bucket_prefix)[0], jenkins_url] |
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.
What is this code doing? I don't quite understand 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.
I added a comment. Does that help?
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.
Yeah I mean I get what you are doing. I just don't really understand how the code achieves the goal. The [bucket, jenkins_url]
on a line by itself confuses me.
I'm not too worried about it, though.
3faf395
to
80225c7
Compare
This dates back from the developer pipeline days which we've ripped out. This isn't really usable as is right now so remove it too. Devs can now easily specify a different S3 bucket via the config.
They're all already gated on `tryWithMessagingCredentials()`. If the credentials are present to emit fedmsgs, then assume we want to emit them.
We're moving to a model where if the right credentials exist, we assume that the pipeline is permitted to affect the outside world (e.g. push containers, import OSTree commits, etc...). In a future commit, we'll add safeguards to ensure that the canonical external endpoints cannot be pushed to except from the official Jenkins. But we don't need to dynamically support both ways in the pipeline.
I'd like to get rid of the `official` variable, and this is the final user of it. For now, hack around this by adding another config knob. In the future, I think there's different ways we could tackle this. The simplest would be to move the versionary script into the pipecfg repo, and then the pipeline automatically uses that script if it's found (though it would require an extra `checkout scm`). Another approach is lowering this into cosa/rpm-ostree and f-c-c somehow, but it needs some design work. It depends also if we want to keep the property of developer builds keeping the `dev` marker.
The logic is functionally equivalent. Prep for next patch.
As we start providing more flexible configuration via the pipecfg, it becomes easier to make a mistake and push things to unintended places. Add a hardcoded list of "protected" Jenkins instances and S3 buckets and add some sanity-checks around bucket and hotfix handling.
This is tested now! |
if (pipecfg.s3.bucket in protected_buckets) { | ||
error("S3 bucket ${pipecfg.s3.bucket} can only be used on ${protected_buckets[pipecfg.s3.bucket]}") | ||
} |
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.
So here we are saying that no one can use the same bucket as a not-hotfix build on these jenkins instances. I imagine they might argue that they want to use the same bucket but a different path within the bucket, which we don't allow for here.
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.
Almost. This is in the non-protected Jenkins instance branch. We're checking that such Jenkins instances don't try to upload artifacts to the canonical S3 buckets.
The hotfix work in the next PR does support writing to the same bucket but different path for hotfix builds. The expectation is that they'll be using the same Jenkins instance for regular and hotfix builds.
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.
oh OK. +1
See individual commit messages.
Requires: #738