Add updater lib #460

joaquimrocha · 2021-09-01T15:55:29Z

I am not entirely sure yet whether we'll keep this here or add it to the go-omaha package. It makes sense to go in the latter, but we need to test some specific behavior in nebraska with it, so for now let's do the PR against a non-main branch.

We also have a gazillion commits because they were initially added continuously to the proof-of-concept branch. I can squash them if needed.

invidian

Some thoughts. Overall code looks straightforward and good 👍 If some of my comments are against general style of the project, please ignore them :)

updater/doc.go

updater/handler.go

updater/omahaHandler.go

updater/updater.go

updater/omahaHandler.go

updater/handler.go

updater/updateInfo.go

invidian · 2021-09-01T17:30:05Z

updater/doc.go

+/*
+Package updater aims to simplify omaha-powered updates.


Should we have some license headers?

That's not needed acording to a recent internal message.

updater/updateInfo.go

rata

Hi!

I've been looking at this, decided to center on the user-facing interface/docs first. I'll review code, tests and wording for comments later, after the first round of fixes (please ping me when done! :)). As the PR description doesn't mention what to pay attention to, I decided to focus on the user-facing part. Let me know if you expected me to do something different! Also, no need to list the commits in the PR description if you are not going to mention anything about any commit in particular, github already lists them :)

Overall, it looks quite reasonable, keep up the good work!

I added several comments. Some are regarding what is exported by this pkg (IOW starts with a capital letter), I think there are lot of functions that should not be exported but that wasn't adjusted yet. To not create a lot of noise, I marked only some. Please review them all, I can look at it again in the next review if some has slipped out.

Documentation for exported types/functions is not present. Please add it, it is hard to review without that, I need to look at lot of details to guess it (I'm not familiar with omaha nor nebraska).

But here it goes a more general question about the whole interface: why not have only one pkg exported to the user? Let me elaborate, but bare in mind that, as some doc is missing, I might be doing wrong assumptions here.

But, why don't we want to just expose to the user one type that has the methods:

New()
CheckForUpdates()
HasUpdate()
GetVersion()
ReportProgress()
TryUpdate() --> not sure about this one, but we can leave for now

IMHO, this seems like a simpler interface to use, and doesn't mix together different things AFAIK.

The CheckForUpdates() can cache the latest info (I mean the UpdateInfo type thing) in an non-exported field of the struct, getVersion() and HasUpdate() just check that info that is cached in the struct (and fail otherwise), and ReportProgress() can do what it does today, but then the user reports that it completed okay, it also does the SetInstanceVersion() as it now knows the version it was updated to (the info cached in the struct).

Note I have not added some other helpers (GetURLs(), etc.) that might be needed in my example, I didn't mean to compile that but just to communicate the gist of my thinking :). Also, the getters in go (as I mentioned in a comment) usually follow a different style, I didn't adapt those methods to that and probably many other things. I wanted this to be a simple to understand idea, not mix several changes in one :).

I think having ReportProgress() know the version it was worked on and automatically using SetInstanceVersion() internally is a nice usability improvement (don't get me wrong, I'm not saying it should be fixed this way, but I would like for thit to be improved in some way).

What do you think?

updater/doc.go

updater/updateInfo.go

updater/doc.go

joaquimrocha · 2021-09-08T09:55:45Z

Hi @rata ,

Thanks for your review. I'd like to address your summary's comments here. We may reply to the in-code ones later.
Please find my reply chained below.

[...] Also, no need to list the commits in the PR description if you are not going to mention anything about any commit in particular, github already lists them :)

FYI, the listing is done automatically by opening the PR with gh 🤷

Overall, it looks quite reasonable, keep up the good work!

Thanks! But it's still WIP 🙂

Documentation for exported types/functions is not present. Please add it, it is hard to review without that, I need to look at lot of details to guess it (I'm not familiar with omaha nor nebraska).

We added docs on how to use it. We quickly made the code available as a PR after interest from others, and it's also a good idea not to document all the methods when the signature/behavior may change, or we'd have to redo the API docs.
We should definitely have API docs once we're settled on the signatures.

But here it goes a more general question about the whole interface: why not have only one pkg exported to the user? Let me elaborate, but bare in mind that, as some doc is missing, I might be doing wrong assumptions here.

One pkg as in a "nebraska package" instead of an "update" pkg? There are two reasons:

The Nebraska package and the updater aren't very much related. In fact, we have the updater lib in Nebraska ATM just for making it easier to test with Nebraska, but it makes sense to move the updater to go-omaha.
Even if it goes to go-omaha, this should be considered as a higher-level lib, thus making sense to keep as a different package from nebraska or go-omaha's main pkgs.

But, why don't we want to just expose to the user one type that has the methods:

New()

CheckForUpdates()

HasUpdate()

GetVersion()

ReportProgress()

TryUpdate() --> not sure about this on
e, but we can leave for now

IMHO, this seems like a simpler interface to use, and doesn't mix together different things AFAIK.

It does mix together different things 🙂 . The updater can be thought of as:

The configured machine/instance's data like instance id, instance version, etc.\
The available updates' information (has a new version, URL, packages, etc.).

Beyong knowing about the instance (in order to perform update checks and reports), the lib is stateless, so having information about the update (like "has update") in the updater itself is giving it a state which I am not sure is desired.

The CheckForUpdates() can cache the latest info (I mean the UpdateInfo type thing) in an non-exported field of the struct, getVersion() and HasUpdate() just check that info that is cached in the struct (and fail otherwise), and ReportProgress() can do what it does today, but then the user reports that it completed okay, it also does the SetInstanceVersion() as it now knows the version it was updated to (the info cached in the struct).

Caching the latest update info looks like an early optimization based on a use-case that's not difficult to be implemented by the caller. IOW, it'd create an assumption that I am not sure is desired for everyone.

About setting the instance version automatically, maybe it does make sense to automatically update the instance version based on a given version number, but this is done in the Updater struct as that's the one holding the minimal state we have (the instance's info).

Note I have not added some other helpers (GetURLs(), etc.) that might be needed in my example, I didn't mean to compile that but just to communicate the gist of my thinking :). Also, the getters in go (as I mentioned in a comment) usually follow a different style, I didn't adapt those methods to that and probably many other things. I wanted this to be a simple to understand idea, not mix several changes in one :).

Maybe we should use the exported variables in the struct (assuming that's the pattern), but that means we'd be duplicating data, and doesn't allow to have a "dynamic" getter. In any case, this is something we can definitely change later.

I think having ReportProgress() know the version it was worked on and automatically using SetInstanceVersion() internally is a nice usability improvement (don't get me wrong, I'm not saying it should be fixed this way, but I would like for thit to be improved in some way).

We could move the ReportProgress to the UpdateInfo struct or have it take an UpdateInfo, but I really want to avoid caching things based on an assumption that may have deep implications in how flexible the lib is.

Let me know if this makes sense.

invidian

Some more comments :)

updater/doc.go

updater/omahaHandler.go

updater/updater_test.go

rata · 2021-09-08T13:38:56Z

@joaquimrocha

Documentation for exported types/functions is not present. Please add it, it is hard to review without that, I need to look at lot of details to guess it (I'm not familiar with omaha nor nebraska).

We added docs on how to use it. We quickly made the code available as a PR after interest from others, and it's also a good idea not to document all the methods when the signature/behavior may change, or we'd have to redo the API docs.
We should definitely have API docs once we're settled on the signatures.

Fair. I didn't know how of a WIP this was when posted that :)

Note I have not added some other helpers (GetURLs(), etc.) that might be needed in my example, I didn't mean to compile that but just to communicate the gist of my thinking :). Also, the getters in go (as I mentioned in a comment) usually follow a different style, I didn't adapt those methods to that and probably many other things. I wanted this to be a simple to understand idea, not mix several changes in one :).

Maybe we should use the exported variables in the struct (assuming that's the pattern)

No, it is not that. And what you supposed doesn't happen. But let's continue this in the appropriate review comment :)

I think having ReportProgress() know the version it was worked on and automatically using SetInstanceVersion() internally is a nice usability improvement (don't get me wrong, I'm not saying it should be fixed this way, but I would like for thit to be improved in some way).

We could move the ReportProgress to the UpdateInfo struct or have it take an UpdateInfo, but I

Sure, that works too.

really want to avoid caching things based on an assumption that may have deep implications in how flexible the lib is.

Ok, I think this is the main thing: you want this to be more flexible than I thought. I tried to make it more simpler :)

Let me know if this makes sense.

IMHO, both things make sense.

My "simpler to the extreme" proposal was just a more minimalistic updater, removing some boilerplate from users (like the info we pass around, we never want an older nor anything else than the last) to reduce the "cognitive load" on users, while still differentiate the two aspects you mentioned (number 1 from your list will be the config, number 2 is the questions you ask to know about the update that are methods).

Yours has more concepts and probably evolve better with the flexibility you want to have. As long as those usability issues are improved, both are fine for me as a user. Thanks!

updater/updateInfo.go

invidian

Overall, I like the direction this PR is going, the API is now much smaller, which it seems allow even more simplification (as stated in the comments).

updater/omahahandler.go

updater/omahahandler_test.go

updater/updater_test.go

updater/doc.go

updater/omahahandler.go

updater/updater_test.go

joaquimrocha · 2021-09-14T15:58:05Z

I have squashed many of the commits. Want to do a last check beyond what was already discussed @invidian / @rata ?

invidian · 2021-09-14T17:50:12Z

Yes, I'd like to review it again, I'll try to do it by tomorrow.

With formik-material-ui we get error checking and displaying errors with input fields out of the box, we have been using TextField from formik-material-ui at other places it was just not used with the group edit forms, also we also don't need to manually set values for a field name as it's also handled.

invidian · 2021-09-15T10:47:37Z

I'm looking into it right now, but it seems some commits slipped in here related to frontend and backend. Are they needed here?

joaquimrocha · 2021-09-15T11:12:25Z

I'm looking into it right now, but it seems some commits slipped in here related to frontend and backend. Are they needed here?

That's because I likely didn't update the target branch. Give me 1 min.

joaquimrocha · 2021-09-15T11:15:48Z

@invidian I don't know what's going on with Github because it should be showing on the new commits on top of the updater branch.
Please review from the "Add updater base code" commit up.

invidian · 2021-09-15T11:17:11Z

@joaquimrocha perhaps some branch need a rebase?

Fix group edit validations

rata · 2021-09-15T11:18:21Z

@joaquimrocha thanks!

I think I hit this github issue before. If the branches are fine on github, then github sometimes is silly and you can change fix it like this: if you go to the top of this page, click edit, then switch the base branch to something else and save (the commits will not be what you want), but then if you do the same but with th branch that it is now, it will show the proper commits in the PR

joaquimrocha · 2021-09-15T11:23:47Z

@joaquimrocha perhaps some branch need a rebase?

I had rebased it, but it does look like a bug, as Rodrigo mentioned.

This patch adds a new CLI option to override what the URL should be for the packages that the syncer creates. Previously this was only possible with the packagfes host option, but even then it would be automtatically generated from the Nebraska URL option. Thus, with this option we give a lot more flexibility.

If the syncer packages URL that the user overrides as a CLI option contain "{{VERSION}}" or "{{ARCH}}", then they will be replaced by their respective value, from the package.

Allow to override syncer's package URL

invidian

Overall looks very good, I pointed some nits mainly, as the exported API looks really clear now, seems like an easy thing to use, which is very cool!

I'm a bit concerned about the tests though, some cases are missing + large portion of the tests require a database which is considered a red flag, as databases tends to be slow. I think it would be much better if we could mock the responses, so we can to thorough testing, then have some small integration tests actually using the database. If can even be the same test suite, just using different Omaha handler, just one run as unit tests and another one as integration test.

As I don't have a database running right now, I cannot assert how good the coverage is on the updater.

updater/omahahandler.go

updater/omahahandler_test.go

updater/updater_test.go

joaquimrocha · 2021-09-15T16:46:44Z

Overall looks very good, I pointed some nits mainly, as the exported API looks really clear now, seems like an easy thing to use, which is very cool!

I'm a bit concerned about the tests though, some cases are missing + large portion of the tests require a database which is considered a red flag, as databases tends to be slow. I think it would be much better if we could mock the responses, so we can to thorough testing, then have some small integration tests actually using the database. If can even be the same test suite, just using different Omaha handler, just one run as unit tests and another one as integration test.

As I don't have a database running right now, I cannot assert how good the coverage is on the updater.

We're actually thinking of moving the updater completely to the go-omaha project now that it's reviewed. The reason why we kept it here in the beginning was to really make sure we could easily test it with Nebraska as a backend. If we keep some of this testing here, we run the tests in CI as we already do for the backend.

Co-authored by: Joaquim Rocha <joaquim@kinvolk.io>

Co-authored by: Santhosh Nagaraj S <santhosh@kinvolk.io>

joaquimrocha · 2021-09-16T11:13:55Z

Thank you @rata and @invidian for your thorough reviews. And @yolossn for enduring them 🙂
Team work! 🦾

invidian · 2021-09-16T11:30:38Z

@joaquimrocha there seems to be a lot of unresolved conversations still. Do we plan to address them somehow?

joaquimrocha · 2021-09-16T12:45:18Z

I think @yolossn addressed most of the comments, although some remained unresolved. I have resolved a few already but need to focus on something else.
Maybe when @yolossn is back next week he can verify if we have some unaddressed comments and we can do those in a new PR if needed.

joaquimrocha requested review from invidian and rata September 1, 2021 15:55

joaquimrocha changed the base branch from main to updater September 1, 2021 16:22

invidian reviewed Sep 1, 2021

View reviewed changes

invidian reviewed Sep 3, 2021

View reviewed changes

updater/updateInfo.go Outdated Show resolved Hide resolved

yolossn force-pushed the updater-lib branch 7 times, most recently from ac7093e to e3ac717 Compare September 7, 2021 09:17

yolossn requested a review from invidian September 7, 2021 09:18

rata suggested changes Sep 7, 2021

View reviewed changes

invidian reviewed Sep 8, 2021

View reviewed changes

invidian reviewed Sep 10, 2021

View reviewed changes

updater/updateInfo.go Outdated Show resolved Hide resolved

rata reviewed Sep 10, 2021

View reviewed changes

updater/updateInfo.go Outdated Show resolved Hide resolved

yolossn force-pushed the updater-lib branch 2 times, most recently from f55e502 to d9d0126 Compare September 13, 2021 04:02

yolossn requested review from invidian and rata September 13, 2021 04:49

frontend: Fix maxCharacters validation message, make name required field

e618d28

yolossn force-pushed the updater-lib branch 3 times, most recently from 026f221 to 1eaf719 Compare September 13, 2021 07:28

invidian reviewed Sep 13, 2021

View reviewed changes

updater/updater_test.go Outdated Show resolved Hide resolved

yolossn force-pushed the updater-lib branch from 1eaf719 to ff194d3 Compare September 13, 2021 10:31

yolossn force-pushed the updater-lib branch from 73a6ae8 to e4d5ac9 Compare September 14, 2021 08:19

yolossn requested a review from invidian September 14, 2021 08:29

joaquimrocha force-pushed the updater-lib branch from e4d5ac9 to a74bd33 Compare September 14, 2021 15:56

Merge pull request #470 from kinvolk/fix-group-edit-validations

4aac09a

Fix group edit validations

joaquimrocha added 5 commits September 15, 2021 12:54

backend: Add testing for some syncer functions

940345f

backend: Replace the version/arch keyswords in syncer packages' URL

71a1560

If the syncer packages URL that the user overrides as a CLI option contain "{{VERSION}}" or "{{ARCH}}", then they will be replaced by their respective value, from the package.

docs: Mention the new syncer-packages-url CLI option

92873e1

Merge pull request #448 from kinvolk/sync_pkg_url

916dcec

Allow to override syncer's package URL

invidian reviewed Sep 15, 2021

View reviewed changes

yolossn and others added 3 commits September 15, 2021 18:02

Add updater base code

0b75772

Co-authored by: Joaquim Rocha <joaquim@kinvolk.io>

updater: Add first tests

b404efe

Co-authored by: Santhosh Nagaraj S <santhosh@kinvolk.io>

updater: Add initial docs

f9978d5

Co-authored by: Santhosh Nagaraj S <santhosh@kinvolk.io>

joaquimrocha force-pushed the updater-lib branch from 36a50bb to f9978d5 Compare September 15, 2021 17:02

joaquimrocha merged commit f2747e3 into updater Sep 16, 2021

joaquimrocha deleted the updater-lib branch September 16, 2021 11:13

mkilchhofer mentioned this pull request Dec 16, 2023

feat: Test updater library #700

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add updater lib #460

Add updater lib #460

joaquimrocha commented Sep 1, 2021 •

edited by yolossn

Loading

invidian left a comment

invidian Sep 1, 2021

joaquimrocha Sep 16, 2021

rata left a comment •

edited

Loading

joaquimrocha commented Sep 8, 2021

invidian left a comment

rata commented Sep 8, 2021

invidian left a comment

joaquimrocha commented Sep 14, 2021

invidian commented Sep 14, 2021

invidian commented Sep 15, 2021

joaquimrocha commented Sep 15, 2021

joaquimrocha commented Sep 15, 2021 •

edited

Loading

invidian commented Sep 15, 2021

rata commented Sep 15, 2021 •

edited

Loading

joaquimrocha commented Sep 15, 2021

invidian left a comment

joaquimrocha commented Sep 15, 2021

joaquimrocha commented Sep 16, 2021

invidian commented Sep 16, 2021

joaquimrocha commented Sep 16, 2021

Add updater lib #460

Add updater lib #460

Conversation

joaquimrocha commented Sep 1, 2021 • edited by yolossn Loading

invidian left a comment

Choose a reason for hiding this comment

invidian Sep 1, 2021

Choose a reason for hiding this comment

joaquimrocha Sep 16, 2021

Choose a reason for hiding this comment

rata left a comment • edited Loading

Choose a reason for hiding this comment

joaquimrocha commented Sep 8, 2021

invidian left a comment

Choose a reason for hiding this comment

rata commented Sep 8, 2021

invidian left a comment

Choose a reason for hiding this comment

joaquimrocha commented Sep 14, 2021

invidian commented Sep 14, 2021

invidian commented Sep 15, 2021

joaquimrocha commented Sep 15, 2021

joaquimrocha commented Sep 15, 2021 • edited Loading

invidian commented Sep 15, 2021

rata commented Sep 15, 2021 • edited Loading

joaquimrocha commented Sep 15, 2021

invidian left a comment

Choose a reason for hiding this comment

joaquimrocha commented Sep 15, 2021

joaquimrocha commented Sep 16, 2021

invidian commented Sep 16, 2021

joaquimrocha commented Sep 16, 2021

joaquimrocha commented Sep 1, 2021 •

edited by yolossn

Loading

rata left a comment •

edited

Loading

joaquimrocha commented Sep 15, 2021 •

edited

Loading

rata commented Sep 15, 2021 •

edited

Loading