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

[JENKINS-73894] Un-inline JavaScript in IssuesChartPortlet/portlet.jelly #1859

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

yaroslavafenkin
Copy link
Contributor

https://issues.jenkins.io/browse/JENKINS-73894
Makes the plugin a step closer to being CSP compatible.

Testing done

I've installed https://plugins.jenkins.io/dashboard-view/ plugin so that I can reach the code in question. Created a new dashboard view, and in Portlets at the top of the page section I've added Static analysis issues chart. I've not spent time setting up a project that would have actual violations, so my issues chart is empty.

https://www.loom.com/share/9ed2252e9eda40e4a17e8e745757fe18?sid=e90705e4-c41c-4b35-9a20-0bf361676310

https://www.loom.com/share/7a2a6a2cab2d428f8aa8a3af873174a0?sid=8cee3380-af6b-44eb-aff7-a352d126c549

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
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

Copy link

codecov bot commented Oct 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.57%. Comparing base (e659c7a) to head (b581ab2).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #1859   +/-   ##
=========================================
  Coverage     82.57%   82.57%           
  Complexity     1360     1360           
=========================================
  Files           249      249           
  Lines          5233     5233           
  Branches        402      402           
=========================================
  Hits           4321     4321           
  Misses          802      802           
  Partials        110      110           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

}
</script>
<st:bind value="${it}" var="trendProxy"/>
<f:invisibleEntry>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this could be changed to have inline style on the span hiding the element if someone is opposed to the current approach.

@basil basil requested a review from uhafner October 14, 2024 14:50
@yaroslavafenkin
Copy link
Contributor Author

The build does not deploy incrementals, does it? Unsure how I can run ATH with this included.

@uhafner uhafner added the internal Internal changes without user or API impact label Oct 15, 2024
Copy link
Member

@uhafner uhafner left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@uhafner
Copy link
Member

uhafner commented Oct 15, 2024

The build does not deploy incrementals, does it? Unsure how I can run ATH with this included.

No, unfortunately incrementals is somewhat limited and does not work for multi-module projects.

Actually, this plugin already contains ATH tests in https://github.com/jenkinsci/warnings-ng-plugin/tree/main/ui-tests. Each change will automatically evaluated against those ATH tests.

Maybe we can add in a later point an ATH test directly in the plugin that ensures CSP now and in the future.

@uhafner
Copy link
Member

uhafner commented Oct 15, 2024

I deployed the change locally and it looks good.

@uhafner uhafner merged commit 0462a87 into jenkinsci:main Oct 15, 2024
43 of 44 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Internal changes without user or API impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants