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

Add commit hash parameter #117

Merged
merged 1 commit into from
Jan 24, 2025
Merged

Add commit hash parameter #117

merged 1 commit into from
Jan 24, 2025

Conversation

azbeleva
Copy link
Contributor

No description provided.

@henrirosten
Copy link
Collaborator

@azbeleva: Have you tested this? Can you add a link to the test run if you did?

utils.groovy Outdated
@@ -205,6 +205,7 @@ def ghaf_hw_test(String flakeref, String device_config, String testset='_boot_')
def imgdir = find_img_relpath(flakeref, 'archive')
def remote_path = "artifacts/${env.ARTIFACTS_REMOTE_PATH}"
def img_url = "${env.JENKINS_URL}/${remote_path}/${imgdir}"
def commit_hash = (img_url =~ /commit_([a-f0-9]{40})/)[0][1]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you could make this a bit less hacky by reading the commit hash from environment variable TARGET_COMMIT, which seems to be set on all the pipelines that potentially call this function e.g. here, here, here, and here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So the line 208 would change to something like:

def commit_hash = "${env.TARGET_COMMIT}"

@azbeleva azbeleva force-pushed the add_commit_hash branch 6 times, most recently from 5822b67 to 4b6b21d Compare January 21, 2025 13:39
@azbeleva azbeleva force-pushed the add_commit_hash branch 2 times, most recently from c04346b to 60ded58 Compare January 23, 2025 22:45
Signed-off-by: Mariia Azbeleva <[email protected]>
@azbeleva azbeleva marked this pull request as ready for review January 24, 2025 09:36
@@ -44,11 +44,13 @@ def ghaf_robot_test(String testname='boot') {
try {
// Pass variables as environment variables to shell.
// Ref: https://www.jenkins.io/doc/book/pipeline/jenkinsfile/#string-interpolation
env.COMMIT_HASH = "${params.COMMIT_HASH}"
Copy link
Collaborator

@henrirosten henrirosten Jan 24, 2025

Choose a reason for hiding this comment

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

What if params.COMMIT_HASH is missing?

You might want to add a check for such case:

if(!params.containsKey('COMMIT_HASH')) {
  println "Missing COMMIT_HASH parameter"
  # Can we continue exexution if COMMIT_HASH is not set?
  # If not, you probably want to fail the build here, e.g.:
  sh "exit 1"
}

Copy link
Contributor Author

@azbeleva azbeleva Jan 24, 2025

Choose a reason for hiding this comment

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

we can continue if it is missing, but can it actually be missing if it is provided in utils.groovy ( string(name: "COMMIT_HASH", value: "$commit_hash") )? it can be empty maybe or the error will happen somewhere else?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's possible ghaf-hw-test pipeline would be triggered from somewhere else too, not only from that one function in utils.groovy. Although, currently that's not the case.

You decide if you want to add such check or not. I'm fine either way.

@azbeleva azbeleva merged commit f9b8754 into main Jan 24, 2025
1 check passed
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.

2 participants