Skip to content

[MSHARED-1125] Require Maven args to be provided one by one #41

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
Merged
slawekjaranowski merged 1 commit into from
Sep 7, 2022
Merged

[MSHARED-1125] Require Maven args to be provided one by one #41

slawekjaranowski merged 1 commit into from
Sep 7, 2022

Conversation

@slawekjaranowski
Copy link
Member

@slawekjaranowski slawekjaranowski commented Sep 6, 2022

No description provided.

gnodet
gnodet approved these changes Sep 6, 2022
View reviewed changes
Copy link
Contributor

@gnodet gnodet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.
Fwiw, I have found 32 references to such pattern in maven-integration-testing by looking for \.addCliOption\( "[\S]+ .*". A PR should be raised asap to align those, even before the switch to verifier 2.0.
Maybe we could add a helper method to add several arguments:

    public void addCliOptions( String... options )
    {
        ...
    }

@slawekjaranowski
Copy link
Member Author

slawekjaranowski commented Sep 6, 2022

I will look at integration tests.

michael-o
michael-o requested changes Sep 6, 2022
View reviewed changes
@michael-o
Copy link
Member

michael-o commented Sep 6, 2022

What about invoker.properties?

@michael-o
Copy link
Member

michael-o commented Sep 6, 2022

Looks good to me. Fwiw, I have found 32 references to such pattern in maven-integration-testing by looking for \.addCliOption\( "[\S]+ .*". A PR should be raised asap to align those, even before the switch to verifier 2.0. Maybe we could add a helper method to add several arguments:

    public void addCliOptions( String... options )
    {
        ...
    }

This is a good idea with varargs...

@slawekjaranowski
Copy link
Member Author

slawekjaranowski commented Sep 6, 2022

What about invoker.properties?

it is not used here ... it is for maven-invoker-plugin - I think.

@michael-o
Copy link
Member

michael-o commented Sep 6, 2022

What about invoker.properties?

it is not used here ... it is for maven-invoker-plugin - I think.

Right, this is used by Invoker.

michael-o
michael-o reviewed Sep 6, 2022
View reviewed changes
michael-o
michael-o reviewed Sep 6, 2022
View reviewed changes
michael-o
michael-o reviewed Sep 6, 2022
View reviewed changes
@michael-o
Copy link
Member

michael-o commented Sep 7, 2022

@slawekjaranowski FYI: https://issues.apache.org/jira/browse/MSHARED-1129

@slawekjaranowski slawekjaranowski merged commit f2ff914 into master Sep 7, 2022
@slawekjaranowski slawekjaranowski deleted the MSHARED-1125 branch September 7, 2022 05:54
@slawekjaranowski slawekjaranowski added the breaking Pull requests that break existing features label Sep 19, 2022
@jira-importer
Copy link

jira-importer commented Jun 9, 2025

Resolve #166

1 similar comment
@jira-importer
Copy link

jira-importer commented Jun 9, 2025

Resolve #166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gnodet gnodet gnodet approved these changes

@michael-o michael-o michael-o approved these changes

Assignees

No one assigned

Labels

breaking Pull requests that break existing features

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@slawekjaranowski @michael-o @jira-importer @gnodet