-
Notifications
You must be signed in to change notification settings - Fork 523
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
feat: add jenkins support #611
feat: add jenkins support #611
Conversation
It would be great if you could take a quick look at it for suggestions or changes before I write the actual documentation. |
Thanks @sinabakh! Will review this today :) |
This is great! To keep the style of fail_condition similar to post_condition, should we also have this option: See this comment and the following 2 comments about the pros/cons of why we picked that style. |
Thanks for the review @alikhajeh1. Regarding the suggestion, I think it can't be applied to such a config. I mean, posting a comment all the time makes sense but failing a build is not something that somebody wants unless a specific condition is met. So adding the Let me know if I'm missing a point or it would helo maintain the same standard. |
Good point! Makes sense. What would be the behavior if
|
That's exactly how you described 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.
@sinabakh thanks for chipping away at this! it's in good shape, just a few minor comments
scripts/ci/jenkins_diff.sh
Outdated
# Set variables based on the order for GitHub Actions, or the env value for other CIs | ||
iac_path=${1:-$iac_path} | ||
terraform_plan_flags=${2:-$terraform_plan_flags} | ||
terraform_workspace=${3:-$terraform_workspace} | ||
usage_file=${4:-$usage_file} | ||
config_file=${5:-$config_file} | ||
fail_condition=${7:-$fail_condition} |
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 think this can be deleted since env vars are used to run the script (not params passed into the script like GH Action).
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.
Sure.
scripts/ci/jenkins_diff.sh
Outdated
if [ ! -z "$fail_condition" ] && [ "$(echo "$fail_condition" | jq '.percentage_threshold')" != "null" ]; then | ||
fail_percentage_threshold=$(echo "$fail_condition" | jq -r '.percentage_threshold') | ||
fi | ||
fail_percentage_threshold=${fail_percentage_threshold:--1} |
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.
hmm I wasn't expecting this to default to -1 since for other CI integrations this is the absolute percentage threshold:
"For example, set to 1 to post a comment if the cost estimate changes by more than plus or minus 1%."
Could we do something similar here? I'm actually not sure how useful the drops in cost posting a comment or failing the build are but I lean towards keeping thins consistent until we have more feedback 🤷♂️
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 I see, I actually had a different intention. Since setting the default value to 0 will collide with "Fail if there was any change in diff (setting the threshold to 0)", I set its default value as -1. Do you have any suggestions to enhance 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.
So we'd want to handle 2 cases:
- never fail the build. Default behavior. Maybe we can do that by checking if
fail_percentage_threshold
is not present? - fail the build if the costs increase or decrease by X%, can be done using the fail_percentage_threshold
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.
Makes sense, done.
scripts/ci/jenkins_diff.sh
Outdated
} | ||
|
||
build_output_cmd () { | ||
breakdown_path=$1 |
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.
@aliscott I saw this line in diff.sh and atlantis_diff.sh too, is it redundant since it's not used in this function or am I missing something?
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.
Removed it from this script but left the Atlantis for another pr.
@alikhajeh1 All done. |
The
jenkins_diff.sh
does relatively the same task as the other diff scripts, here is a list of changes:path
variable is changed toiac_path
post_condition
fail_condition
with thepercentage_threshold
field to fail the pipeline if the change is over the thresholdHere's a sample
Jenkinsfile
config:Here's the pipeline's "Output consone":
And the output get's its own sub-menu: