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

[MINVOKER-250] streamLogsOnFailures #20

Merged
merged 1 commit into from
May 20, 2020
Merged

[MINVOKER-250] streamLogsOnFailures #20

merged 1 commit into from
May 20, 2020

Conversation

slawekjaranowski
Copy link
Member

No description provided.

@@ -103,6 +103,13 @@ under the License.
<type>int</type>
<description>BuildJobs will be sorted in the descending order of the ordinal. In other words, the BuildJobs with the highest numbers will be executed first</description>
</field>
<field xml.attribute="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

Is xml.attribute something modello has already defined? They shouldn't have. Names beginning xml are reserved in the XML 1.0 spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

Another fields in this file is defined in the same way.

As I see xml.attribute="true" has effect that field is added as xml attribute instead of new element/tag in xml.

Copy link
Member Author

Choose a reason for hiding this comment

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

pom.xml Outdated Show resolved Hide resolved
@slawekjaranowski
Copy link
Member Author

PR was rebased with current master branch.
Code was corrected according to comments from review.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

I might try to shorten the name of the option. E.g. instead of streamLogsOnFailures perhaps simply logFailures

@slawekjaranowski
Copy link
Member Author

I wanted to name this option similar to one existing streamLogs.
I think that we will also change connected jira issue, so we should be consistent .

I'm waiting for decision.

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

It occurred to me last night that since this is a user facing property, there are likely some docs that need to be updated in this PR as well. A property doesn't help if no one can find it.

@slawekjaranowski
Copy link
Member Author

Which document do you think should be updated?

I added javadocs comments on new option, so it will be available on standard mojo help, on site and by command line.

@elharo
Copy link
Contributor

elharo commented May 19, 2020 via email

@olamy
Copy link
Member

olamy commented May 20, 2020

@slawekjaranowski Thanks for this PR this new feature will be very helpful!! Maybe you can can add a page in examples section something very simple similar to https://maven.apache.org/plugins/maven-invoker-plugin/examples/skipping.html

@slawekjaranowski
Copy link
Member Author

Documentation added in exemples.

@olamy olamy merged commit 1c55108 into apache:master May 20, 2020
@slawekjaranowski slawekjaranowski deleted the fix-250 branch May 20, 2020 21:04
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.

4 participants