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

Repository Browser: Add context menu action "Update Revisions :: To higher MICRO / MINOR / MAJOR revision" #6104

Merged
merged 11 commits into from May 7, 2024

Conversation

chrisrueger
Copy link
Contributor

@chrisrueger chrisrueger commented Apr 27, 2024

This PR is a suggestion for bnd's MbrCommand e.g. bnd mbr update -s micro but making it available in bndtools' UI.

This adds a context menu when you Right-Click on a MavenBndRepo like this:

image

This allows to increase versions of a full central.mvn maven index file.

Similar functionality already exists if you right-click on a single RepoBundle-Revision. This suggestion here basically does it for the whole repository.

I added a Dry-Run-Option which does not modify the .mvn file, but instead copies the result to the Clipboard.
I am not sure if this is a good idea, since it can take some time depending on the size of the repo and the user might not notice it... UX isn't great. I kept it for now, since it helped during debugging, but I am fine with removal too... or suggestions how it could be improved. Better wording suggestions welcome too.

Also: I basically copy pasted and adjusted the code from MbrCommand. There is room for better re-use, but I just wanted to prototype this first.

Dry-Run

The output of the Dry-Run is similar to MbrCommand#_check e.g.:

## Updates available
com.fasterxml.jackson.core:jackson-core:2.16.1 2.16.2
etc.

this basically does what the MbrCommand does, but in the UI. You can update a central.mvn index file with this action.
There is a DryRun option, which just copies the result to clipboard without touching the index file. you can use that to simulate it and check the result upfront.

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
and add options for scope MICRO, MINOR and MAJOR
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
now it looks like MbrCommand#_check which is a string table showing the current and new version - basically a preview
e.g.
## Updates available
com.fasterxml.jackson.core:jackson-core:2.16.1                        2.16.2

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
Copy link
Contributor Author

@pkriens Do you have any thoughts? Would this be a useful feature?

How would you go about the code re-use between MbrCommand.java and this class?

I first thought about reusing MbrCommand directly, but then decided I pull out the stuff first (copy paste). Maybe we should put the code from MbrCommand.java into a util class like I have (Mbr.java) and call that from MbrCommand.java and RepoActions.java.

@pkriens
Copy link
Member

pkriens commented May 1, 2024

I agree that the shared code should be shared. This seems a good place. I would also be ok to make it methods on the MavenBndRepository. But I can go either way.

nice work!

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger chrisrueger marked this pull request as ready for review May 1, 2024 20:27
@chrisrueger
Copy link
Contributor Author

Thanks for the feedback @pkriens

I added some commits trying to share as much code as I could.

One question about something I am not sure:
In calculateUpdateRevisions() https://github.com/bndtools/bnd/pull/6104/files#diff-9798d76a25282bc3dff5647b6739f82970d66f20f02645d60e6bad8333fc1b91R86 I added a PrintStream out parameter. But I would rather avoid it. I only did it because previously bnd.out.format(...) was used from MbrCommand. bnd.out seems to be System.out

But there were some other calls to bnd.trace(...) which also seems to call System.out.println() under the hood. The trace() calls I have solved with the Trace logger https://github.com/bndtools/bnd/pull/6104/files#diff-9798d76a25282bc3dff5647b6739f82970d66f20f02645d60e6bad8333fc1b91R36 passed to the constructor.

I was wondering: since both are basically writing to System.out in one way or the other: Could I get rid of the PrintStream out parameter in calculateUpdateRevisions(...) and also use the Trace logger? (logger.trace(...) instead of out.format(...))

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@pkriens
Copy link
Member

pkriens commented May 3, 2024

We can use the logger for tracing etc. Maybe it should use Result for the methods instead of outputing information? In general you do not want to do any output in a component like this.

TODO investigate into suggestion of using Result instead of outputing

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Since MbrUpdater should not print out directly any logging statements, I tried to do it outside in MbrCommand. Because MbrCommand also did the logging before, so I tried do it in a similar way, to preserve existing behavior.

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
Copy link
Contributor Author

We can use the logger for tracing etc. Maybe it should use Result for the methods instead of outputing information? In general you do not want to do any output in a component like this.

Thanks for the suggestion. unfortunately I did not see how aQute.bnd.result.Result would have helped me to log like before in MbrCommand.

Thus I decided a different approach. I pulled out all logging from the new class MbrUpdater and did the logging manually in MbrCommand. See 2d1f949

The logging output is the same, but the place is obviously a bit different than before, e.g. because now I am logging after I have all the results - while before logging happened on the go during iteration in the loop.

Question: Is this an ok way to check for "updated" versions?
2d1f949#diff-9bfdb19b4bf9085bd6f3d2c890803a16eb8b3fe79b3e4f923dd725e8c4cd1d1aR162-R166

@chrisrueger
Copy link
Contributor Author

chrisrueger commented May 3, 2024

Question: Is this an ok way to check for "updated" versions?
2d1f949#diff-9bfdb19b4bf9085bd6f3d2c890803a16eb8b3fe79b3e4f923dd725e8c4cd1d1aR162-R166

Damn, I noticed I misunderstood the previous logging. I thought the previous logging only logged updated versions.
But it just logged when the list was not empty (meaning that there was a version returned from maven).

- Do you think this is ok? (but different logging behavior, because only version > currentVersion is logged)
- or should I change it back to the previous logging behavior, where everything is logged which received a result from maven-central (even though it could be the same version as current version)

Update: Forget the above. I added a new commit. I introduced a new record object in fc97d92 which allows to write the log output the same way as it was before in MbrCommand. The only difference now is that it gets written a bit later after the Map has been built (previously logging happened on the go in the loop).

the new container obj allows to better track when we got a version from the mavencentral-fetch, and this in turn allows us to write the logging output the same way as it was before in MbrCommand

Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
Signed-off-by: Christoph Rueger <chrisrueger@gmail.com>
@chrisrueger
Copy link
Contributor Author

Some notes about how to test bnd mbr from the cli

cd ~/my-bndtools-workspace

## execute bnd mbr update -d  (-d is dry run)
java -jar ~/Downloads/Dist_Bundles_JDK17_ubuntu-latest/biz/aQute/bnd/biz.aQute.bnd/7.1.0-SNAPSHOT/biz.aQute.bnd-7.1.0-20240503.192058-1.jar bnd mbr update -d 

Expected example output:

com.fasterxml.jackson.dataformat:jackson-dataformat-yaml                             2.16.1 -> 2.17.0
com.fasterxml.jackson.jaxrs:jackson-jaxrs-base                                       2.16.1 -> 2.17.0

@pkriens
Copy link
Member

pkriens commented May 7, 2024

LGTM, want to merge it?

@chrisrueger
Copy link
Contributor Author

LGTM, want to merge it?

Yes

@chrisrueger chrisrueger merged commit e7ce501 into bndtools:master May 7, 2024
9 checks passed
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.

None yet

2 participants