[MNG-5913] Allow defining aliases for existing server configurations in settings.xml#2333
[MNG-5913] Allow defining aliases for existing server configurations in settings.xml#2333slawekjaranowski wants to merge 7 commits intoapache:masterfrom
Conversation
|
This needs to be made sure will not break Maven 3, as same settings is shared by both currently. |
7230d9e to
563d2e2
Compare
unit test, looks ok for me. I tested with: |
|
ok, who is willing to work on it with me? I hope such feature will be valuable. |
|
Well, settings with current 3.9.9 are NOT read: Or this is "ok"? Is settings loaded partially or discaded? |
|
I am willing to jump in as Njord does something similar, but my concern is that Maven 3.x is not "forgiving" while readint |
it is correct because 3.9.x does not have it yet |
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
I hope we can have next version of settings schema for 3.9.x ... so if someone update own settings alos will use newer version of Maven |
Pankraz76
left a comment
There was a problem hiding this comment.
already very nice.
please give last dedication, to reach infinitive higher-order function.
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java
Show resolved
Hide resolved
...lder/src/test/java/org/apache/maven/settings/building/DefaultSettingsBuilderFactoryTest.java
Outdated
Show resolved
Hide resolved
a959fb0 to
e0b3b6b
Compare
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
|
@gnodet @Pankraz76 last suggestions applied |
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Show resolved
Hide resolved
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
...ettings-builder/src/main/java/org/apache/maven/settings/building/DefaultSettingsBuilder.java
Outdated
Show resolved
Hide resolved
|
why we need to change deprecated stuff? PMD or rewrite, might even do both. |
deprecated staff can be used by Maven 3 plugins - we need support backward compatibility for Maven 3.x |
|
I am still unconvinced that this change is backward compatible. Or the idea is that whoever uses this feature must use Maven 4 or latest (upcoming) Maven 3.9.10? What happens if by mistake (or project mvnw) is 3.9.9 or 3.8.8? |
Confirm - for new future Maven 4.x (rc-4+) and 3.9.10+ will be needed. Such futures will be needed for deployments or for special environments where users use more private repositories. For older Maven warning will be printed. |
I am aware of warning, but am unaware what exactly happens when that warning is printed: is whole settings ignored, is only the server (with unknown element) ignored... Given we talk about same file that all Maven 3+ would use, I think we need to be careful here. Maven is already printing a LOT to screen, is very easy to miss one line warning and then tinker why build/deploy/whatever failed... |
|
As I see from my testing only unparsable settings.xml break a Maven build at all I can add IT for such case - what happens with unrecognized tag in settings.xml - in separate PR if can help you |
5167598 to
c726fc1
Compare
|
Kindly reminder - Any more comments here? Anything what I can do more ...? If there are no more requests, I would like to merge it |
There was a problem hiding this comment.
I consider it as confusing having <id/> and <ids/> as subelements. I think this needs to be in Maven 4 first, RC and then in 3.9.x. I think that one server element should have a unique ID and then <aliases/>.
There is also a JIRA issue which requests to completely decouple credentials from server elements. Which makes creds reusable throughout.
|
Looking at this I am still convinced that is should not be delivered with 3.9.10. |
|
Yet another problem is that we are abusing one server structure for multiple ones, e.g., you are settings headers or other transport-related config. It will be applied to mulitple servers, but you need to split them up actually except credentials. |
|
Resolve #7757 |
…in settings.xml Add the next tag ids to the server in settings.xml Additional server will be created in memory by SettingsBuilder, so the generated configuration should be transparent to other as Settings#getServers() will return complete list.
c726fc1 to
d634949
Compare
|
@michael-o @cstamas I refreshed a PR 😄 with last suggestions to build pass I need fixes in modello - in progress |
|
Will check next week... |
| <version>1.3.0+</version> | ||
| <description> | ||
| List of additional server aliases. For each alias, an additional server will be generated by copying the entire configuration, but using a different ID. | ||
| This is useful when the same credentials should be used for multiple servers. </description> |
| .id(id) | ||
| .aliases(List.of()) | ||
| .build()); | ||
| } |
There was a problem hiding this comment.
This looks very ugly. It is the clien't responsibility to traverse the Settings object, no?
There was a problem hiding this comment.
It is the core of change.
Based on additional tag - aliases we create a next server instance in memory on server list.
So all others plugins tools will see destination list of servers original declared in settings and created based on aliases tag.
There was a problem hiding this comment.
I do remember that Modello allows to add Java code to Settings. Why can't we inject/improve the code in the model directly?
There was a problem hiding this comment.
so you propose to override a getServers method directly in modelo?
There was a problem hiding this comment.
ok, I will check if it will be easier?
There was a problem hiding this comment.
@michael-o
It will be impossible (in easy way) to implement in code settings.mdo, we use velocity templates to generate code for 4.x and for 3.x.
There was a problem hiding this comment.
Share the spot where this is done. Modello made it possible :-(
There was a problem hiding this comment.
Share the spot where this is done. Modello made it possible :-(
There is two templates for generating output from modello files:
https://github.com/apache/maven/blob/master/src/mdo/model.vm
https://github.com/apache/maven/blob/master/src/mdo/model-v3.vm
I see some of code in settings.mdo so will look again
There was a problem hiding this comment.
I don't see a way to provide such code in settings.mdo
Add the next tag ids to the server in settings.xml
Additional server will be created in memory by SettingsBuilder, so the generated configuration should be transparent to other as Settings#getServers() will return complete list.
https://issues.apache.org/jira/browse/MNG-5913