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

add --update option #693

Merged
merged 5 commits into from
Feb 26, 2024
Merged

add --update option #693

merged 5 commits into from
Feb 26, 2024

Conversation

NicolasKeita
Copy link
Contributor

@NicolasKeita NicolasKeita commented Oct 16, 2023

add --update option (or -u) to enable comparing the user's version of each framework to the latest version available.

Inspired by an old issue #74 and I wanted to help.

Side nodes:

  • This new feature does not fully automate the update of all outdated packages (it proposes to the user to only update one package) but presents a clear indication of which frameworks are outdated (there are rooms for improvements).
  • I did not create test-cases for this feature but I didn't break any previous successful tests neither. (There are rooms for improvements).

Feature details:

  • I get the framework's latest version from parsing the package url.
  • I get the current user version of the framework by reading files in the install_path (most of the times, there is a version.txt file. I handle it differently for each framework).
  • Then I compare and display which framework is outdated in the terminal as shown in the image below.

image

@LyzardKing
Copy link
Collaborator

LyzardKing commented Oct 27, 2023

Hi. Thanks for contributing this.
The update functionality has been on my mind for a while.
Unfortunately I don't have too much time to check this, but I'll get to it soon.

Just a first few suggestions:

  • I would avoid printing everything, and instead only show the updatable items.
  • keep the default "Missing Information" functions in baseInstaller, and just override if needed.

In a previous attempt I started a while back I added an argument (updatable) to distinguish the frameworks that shouldn't be updatet through umake (e.g. those that have their own updater).

@LyzardKing
Copy link
Collaborator

You can find the old version of the update function here:
#506

Just to have a quick look at the defaults and "updatable" argument.

@LyzardKing
Copy link
Collaborator

Hi @NicolasKeita
I haven't had much time to look into this, but I'm updating a few things (packaging mainly) on the new_snap branch.
I need to simplify/remove most of the tests, since they are not maintained.

Did you manage to look at making the update function more modular?

@NicolasKeita
Copy link
Contributor Author

Hi,

I believe that I approached the task with the mindset of adding my code on top of the existing code rather than modifying it to avoid any potential disruptions.
If the tests could be revised, I would be more confident in improving my pull request.
I believe I can make it more modular :)

@LyzardKing
Copy link
Collaborator

Thank you very much for the patience and willingness to look into this.

My proposal would be to split this in two:

  • add a generic "default" method in frameworks/init.py and frameworks/baseInstaller.py (as in the previous attempt I linked to)

  • override that method only in frameworks that can update via umake (things like intellij should not because they have their own update system)

  • add a --list-upgradable in init.py that shows what you did now, so a list of upgradable frameworks.. I would not format them as a table, or anything fancy, trying instead to reuse most of the list method that exists now.

Let me know what you think, and thank you again!

@NicolasKeita
Copy link
Contributor Author

Thanks.

I understood the proposal. Looks good to me. I'll take your pull request (#506) code as base. I should be submitting something in the next few days.

@LyzardKing
Copy link
Collaborator

Keep in mind that my old pull request is very old now, so it's not up to date. Just look at it for pointers, or rebase that on master. The first one might be easier.

Let me know if you need help with anything!

The changes I'll be making should not interfere with whatever you do, and at the moment they live in different branches.

Copy link
Collaborator

@LyzardKing LyzardKing left a comment

Choose a reason for hiding this comment

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

I gave your code another look,
I think it's great, and the only thing required is to avoid repeating too much code in the single frameworks.

If possible keep the default methods (those that say missing information) in baseinstaller, and the frameworks only override them if they are updatable by umake.

@@ -75,3 +75,10 @@ def post_install(self):
"""Add Crystal necessary env variables"""
add_env_to_user(self.name, {"PATH": {"value": os.path.join(self.install_path, "bin")}})
UI.delayed_display(DisplayMessage(self.RELOGIN_REQUIRE_MSG.format(self.name)))

def parse_latest_version_from_package_url(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Initially you can remove these empty methods, and add them to baseinstaller/init
so the single frameworks only override this if they can

@@ -79,6 +79,18 @@ def post_install(self):
add_env_to_user(self.name, {"PATH": {"value": os.path.join(self.install_path, "bin")}})
UI.delayed_display(DisplayMessage(self.RELOGIN_REQUIRE_MSG.format(self.name)))

def parse_latest_version_from_package_url(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are fine, since they add what is specific to a package, although they could simply return the regex string, and let a method in baseinstaller deal with evaluating the regex

@@ -243,9 +300,52 @@ def main(parser):
print(get_version())
sys.exit(0)

if args.update:
Copy link
Collaborator

Choose a reason for hiding this comment

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

keep this as is, we can evantually replace the prettyprint, but it looks good for now

@staticmethod
def get_current_user_version(install_path):
return None

Copy link
Contributor Author

@NicolasKeita NicolasKeita Jan 10, 2024

Choose a reason for hiding this comment

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

Adding two default methods for each framework.
get_latest_version() is getting the version from parsing the download_url.
get_current_user_version() is overridden in each of the framework. It is checking the installed files (like VERSION.txt for example) to fetch the version.

@NicolasKeita
Copy link
Contributor Author

the pretty_print function can be easily remove if needed. I filtered only the outdated frameworks (compared to printing every framework installed)

Screenshot from 2024-01-10 05-18-05

@@ -153,6 +154,8 @@ def __init__(self, name, description, category, force_loading=False, logo_path=N
self.packages_requirements.extend(self.category.packages_requirements)
self.only_for_removal = only_for_removal
self.expect_license = expect_license
self.version_regex = version_regex
self.forbidden_to_update = False
Copy link
Contributor Author

@NicolasKeita NicolasKeita Jan 10, 2024

Choose a reason for hiding this comment

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

    self.forbidden_to_update = False

This was my way of preventing an update on some frameworks like Intellij.
Set by default at False then overridden in the class BaseJetBrains for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would have used it the other way around, keeping the default as not updatable, and setting a framework to updatable explicitly. This way we only add information for those that have this functionality.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After thinking about it, I 100% agree with you. I have committed these changes 👍

Comment on lines 335 to 340
for outdated_framework in outdated_frameworks:
if outdated_framework['forbidden_to_update'] is False:
args = parser.parse_args([outdated_framework['category_name'], outdated_framework['framework_name']])
CliUI()
run_command_for_args(args)
return
Copy link
Contributor Author

@NicolasKeita NicolasKeita Jan 10, 2024

Choose a reason for hiding this comment

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

If multiple outdated frameworks are present, only the first framework updates (the return statement is early).
I struggled a bit with running CliUI() in a loop.

PS: sorry about the useless/extra commit (Merge branch 'ubuntu:master' into master)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll look into this.
The best way would probably be to relaunch umake, but I'll let you know

Set specific frameworks to be updatable and make non-updatable the default for all other frameworks
@LyzardKing
Copy link
Collaborator

Great!

The only thing missing now is to add a couple of tests. Do you think you could look into that? Specifically a "small" test (you can find them in the tests/small folder), to see if the behaviour is consistent.

I've been looking into the multiple update, and I think I have an idea, but I need to test it out. Basically we should have a way to run umake with multiple frameworks first (for the install as well), then we could reuse that for the update.

@NicolasKeita
Copy link
Contributor Author

Yes, working on it 👍

@NicolasKeita
Copy link
Contributor Author

Hi,

I couldn't add proper tests.
I've faced difficulties with simple tasks, such as initializing a framework object or installing a framework.
I've added a placeholder for tests in the meantime. I don't think I am able to perform the tests 😞

@LyzardKing
Copy link
Collaborator

I'll look at adding a few tests, just to check that future updates don't break everything, then I'll merge.
I might need some time though, since work is taking over at the moment.
I'll keep you posted and hopefully this can be merged soon.

Thanks again!

@LyzardKing LyzardKing changed the base branch from master to update February 26, 2024 12:28
@LyzardKing
Copy link
Collaborator

Merging in a separate branch to check and add tests if possible.
It'll be then merged in master

@LyzardKing LyzardKing merged commit 3f67a87 into ubuntu:update Feb 26, 2024
3 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.

2 participants