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

Allow missing FilePath when expanding macros #241

Merged
merged 1 commit into from
Jan 21, 2025
Merged

Conversation

zbynek
Copy link
Contributor

@zbynek zbynek commented Jan 9, 2025

Sometimes it's useful to run the tm step even if there is no active node/workspace. Instead of checking that FilePath is present upfront, check it only while expanding a macro that needs it.

Testing done

Checked that the Build Failure Analyzer macro expands properly after all connections to nodes are stopped.

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • [n/a] Link to relevant issues in GitHub or Jira
  • [n/a] Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Sorry, something went wrong.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@slide
Copy link
Member

slide commented Jan 9, 2025

Have you done any checking to see if there are token macros from other plugins that would need to be updated? This changes some of the public API, so we would need to make sure this doesn't break anything.

@slide
Copy link
Member

slide commented Jan 9, 2025

The list is auto-generated, so it should be pretty accurate. We'll need to verify that this update works with email-ext at a minimum.

@zbynek
Copy link
Contributor Author

zbynek commented Jan 14, 2025

@slide https://github.com/jenkinsci/email-ext-plugin/pull/576/files contains a test that shows that exception handling in EmailExt got slightly improved by this (before a NPE would be thrown that results in macro not expanding at all, now it expands to a useful error message). The other PR is still a draft because it depends on incremental build of this one, if this one is merged and released I'll update the other one.

@zbynek
Copy link
Contributor Author

zbynek commented Jan 18, 2025

@slide can I do something to help get this merged?

@slide
Copy link
Member

slide commented Jan 18, 2025

Let me take a look into it more on Monday. I just want to make sure that we don't break anyone.

@slide slide merged commit c36312f into jenkinsci:main Jan 21, 2025
17 checks passed
@zbynek zbynek deleted the filepath branch January 21, 2025 21:37
@zbynek
Copy link
Contributor Author

zbynek commented Feb 1, 2025

@slide could you please cut a release with the fix? My understanding of CD is that this PR needs a label and then another run on the default branch is needed, e.g. triggered by merging a housekeeping PR like this one.

@mwebber
Copy link

mwebber commented Feb 3, 2025

@zbynek @slide This doesn't work with email-ext.
As in "my jobs works with Token macro 400.v35420b_922dcb_, fails when I update to 442.v4f452dc3c7c0, and works again when I fall back.

It was doing this:

emailext(
  to: "${EMAIL_REPORT}",
  subject: "${enviroment_prefix}Asset Tracker New & Updated Resources ${start_date.toString()} to ${end_date.toString()}",
  mimeType: 'text/html',
  body: '${FILE,path="asset-tracker/resources_report_body.html"}',
)

The error reported in my pipeline was Error evaluating token: Error processing tokens
And it sends an email with the body containing the literal string ${FILE,path="asset-tracker/resources_report_body.html

@slide
Copy link
Member

slide commented Feb 3, 2025

@mwebber Thanks for the report, I'll take a look ASAP.

@zbynek
Copy link
Contributor Author

zbynek commented Feb 3, 2025

@mwebber sorry, hopefully fixed in #249

@zbynek zbynek mentioned this pull request Feb 3, 2025
5 tasks
@mwebber
Copy link

mwebber commented Feb 4, 2025

@mwebber sorry, hopefully fixed in #249

Thanks for the quick fix @zbynek . I'll test as soon as it's released.

@zbynek
Copy link
Contributor Author

zbynek commented Feb 4, 2025

@mwebber you can also download the plugin as .hpi from https://ci.jenkins.io/job/Plugins/job/token-macro-plugin/job/main/ and upload it to your instance via "adv (but I'm sure it will be released soon)

@mwebber
Copy link

mwebber commented Feb 4, 2025

@zbynek I tested with the just-released 444.v52de7e9c573d (with #249) and I can confirm that it fixed the problem for me.

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.

None yet

3 participants