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.

Sign up for GitHub

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

Jump to bottom

MSITE-842 #8

Closed
wants to merge 6 commits into from
Closed

MSITE-842 #8

wants to merge 6 commits into from

Conversation

bertysentry
Copy link
Contributor

  • Ensured that the actual filename of the Sink being rendered is exposed in RenderingContext for multi-page reports
  • Sub-pages (or sub-sinks) in subdirectories are now supported

@bertysentry
MSITE-842
474320a 
* Ensured that the actual filename of the Sink being rendered is exposed in RenderingContext for multi-page reports
* Sub-pages (or sub-sinks) in subdirectories are now supported
@michael-o
Copy link
Member

Looking into it.

michael-o
michael-o reviewed Jun 26, 2019
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

You have several formatting shifts. Can you review and fix? I wll try meanwhile to understand your changes.

@michael-o michael-o self-requested a review June 26, 2019 20:04
michael-o
michael-o requested changes Jun 26, 2019
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

You have several formatting shifts. Can you review and fix?

@michael-o
Copy link
Member

The fix is fine, the IT passes, Default Skin and Fluido Skin pass also. You just need to fix the formatting and I will merge and reroll 3.8.1.

@bertysentry
Copy link
Contributor Author

Yeah, sorry about the formatting. I don't know how I missed this...

@bertysentry
MSITE-842
8b09fed 
* Fixed format issues after code review
@bertysentry
MSITE-842
b20c24b 
* Undone unintentional changes on whitespaces
@bertysentry
Copy link
Contributor Author

All fixed! Sorry for the double commit, there were still some whitespaces changes that were unintentional and unnecessary. Let me know if you prefer me to squash everything.

@bertysentry
MSITE-842
0205e0a 
* Fixed silly issue introduced by git reverse
michael-o
michael-o requested changes Jun 27, 2019
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

A few issues with the formatting left and then we can merge.

src/it/projects/MSITE-842/src/site/show-properties.vm Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/main/java/org/apache/maven/plugins/it/MyReport.java Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/main/java/org/apache/maven/plugins/it/MyReport.java Outdated Show resolved Hide resolved
@bertysentry
MSITE-842
8815839 
* Fixed remaining formatting issues in the code of the new IT (MSITE-842)
@bertysentry
Copy link
Contributor Author

All requested changes implemented. I hope I didn't introduce other problems...

michael-o
michael-o requested changes Jun 27, 2019
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

Still some issue left, please see comments

src/it/projects/MSITE-842/pom.xml Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/pom.xml Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/it/MSITE-842.html Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/it/another-page.html Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/it/sub/sub.html Outdated Show resolved Hide resolved
src/it/projects/MSITE-842/src/site/show-properties.vm Outdated Show resolved Hide resolved
@bertysentry
MSITE-842
fbf482b 
* Fixed SLA formatting
* Fixed IT **pom.xml** indentation
asfgit pushed a commit that referenced this pull request Jun 27, 2019
@bertysentry @michael-o
[MSITE-842] $currentFileName and $alignedFileName are incorrect for M…
4d863ed 
…aven Report plugins that use several Sink instances

This closes #8
* Ensured that the actual filename of the Sink being rendered is exposed in RenderingContext for multi-page reports
* Sub-pages (or sub-sinks) in subdirectories are now supported
asfgit pushed a commit that referenced this pull request Jun 27, 2019
@bertysentry @michael-o
[MSITE-842] $currentFileName and $alignedFileName are incorrect for M…
512abf5 
…aven Report plugins that use several Sink instances

This closes #8
* Ensured that the actual filename of the Sink being rendered is exposed in RenderingContext for multi-page reports
* Sub-pages (or sub-sinks) in subdirectories are now supported
@michael-o michael-o self-requested a review June 30, 2019 08:58
michael-o
michael-o requested changes Jun 30, 2019
View reviewed changes
Copy link
Member

@michael-o michael-o left a comment

Choose a reason for hiding this comment

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

All is fine now, expect for some jira on the test runner. I need to figure out why and then I can merge. Any ideas?

@bertysentry
Copy link
Contributor Author

@michael-o What do you see in the test runner? I get all tests as "PASSED" in the integration tests (and unit tests as well).

@michael-o
Copy link
Member

See for yourself: https://builds.apache.org/view/M-R/view/Maven/job/maven-box/job/maven-site-plugin/job/MSITE-842/3/console

@bertysentry
Copy link
Contributor Author

That's weird. The generated MSITE-842.html file must equal to the one provided in src/it. In Jenkins logs, they appear to be the same, but Groovy is saying that they are not.

Maybe it's a Windows-vs-UNIX end-of-line issue, although I would have expect the File.getText() Groovy method to handle these gracefully.

Jenkins seems to be running on Windows here. Maybe the source code was obtained with UNIX eol and the produced file is using Windows eol. Or the opposite, go figure... Do you have access to the actual files on Jenkins?

@michael-o
Copy link
Member

I assume line ending is the case. Unfortunately, I don't have access to the Jenkins workspaces. I only see the physical paths.

@michael-o
Copy link
Member

I will try to remove the license header and see how far I get.

@michael-o
Copy link
Member

OK, dropped the license headers, let's see what happens.

asfgit pushed a commit that referenced this pull request Jul 1, 2019
@bertysentry @michael-o
[MSITE-842] $currentFileName and $alignedFileName are incorrect for M…
7d3bb8c 
…aven Report plugins that use several Sink instances

* Ensured that the actual filename of the Sink being rendered is exposed in RenderingContext for multi-page reports
* Subpages (or subsinks) in subdirectories are now supported

This closes #8
@michael-o
Copy link
Member

RAT breaks the build. We need to figure out the failure and fix it.

@michael-o
Copy link
Member

As far as I remember I had a similar case with some other components in ITs. I normalized the line endings because Groovy's getText() did not normalize line endings.

@asfgit asfgit closed this in 42617d0 Jul 1, 2019
@bertysentry
Copy link
Contributor Author

@michael-o I see the build was successful. How did you fix it? (and thank you for merging! :-) )

@michael-o
Copy link
Member

@bertysentry see: 42617d0#diff-cd32b8e8548e1b3e534123783da08916R29

I will try to roll today or tomorrow 3.8.1

@bertysentry
Copy link
Contributor Author

Ok thank you for fixing the problem, and sorry about that.
And thank you for 3.8.1! :-)

@bertysentry bertysentry deleted the feature/MSITE-842 branch July 3, 2019 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michael-o michael-o michael-o requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bertysentry @michael-o