Skip to content

Upgrade module#32

Merged
zaibon merged 14 commits into
masterfrom
upgrade
Jun 14, 2019
Merged

Upgrade module#32
zaibon merged 14 commits into
masterfrom
upgrade

Conversation

@zaibon

@zaibon zaibon commented May 21, 2019

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread specs/upgrade/README.md
Comment thread specs/upgrade/README.md
@zaibon zaibon force-pushed the upgrade branch 2 times, most recently from 9dd05a9 to b3fce77 Compare May 29, 2019 07:32
@zaibon zaibon self-assigned this May 29, 2019
@zaibon zaibon changed the title add specs for upgrade Upgrade module Jun 6, 2019
@zaibon zaibon marked this pull request as ready for review June 6, 2019 09:55
@zaibon zaibon requested a review from muhamadazmy June 6, 2019 09:56
Comment thread modules/upgrade/file.go Outdated
"path/filepath"
)

var ModeExecutable os.FileMode = 0111

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the correct filemode should be 0100, since the permissions are not actually a hierarchy (assuming it is an executable and not a script, since a script needs to be readable too iirc)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed, the last 1 is probably not relevant. 0110 is enough I think, only test if the current user or group can execute ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Don't think that is 100% correct either, since it will mark a file which as 0010 permissions as executable, but if the file is owned by your user, it is actually not. Tested this a bit locally, and a file owned by my user can indeed not execute some other file I own if its marked as 0077.

This answer on stackexchange seems to give a pretty detailed explanation https://unix.stackexchange.com/a/411901

Comment thread modules/upgrade/publisher.go Outdated
Comment thread modules/upgrade/upgrade.go Outdated

// FIXME: not sure about the public interface of this package yet
// most probably will need to run as a daemon too
func (u *UpgradeModule) Run(period time.Duration, p Publisher) error {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It seems this function never returns, so this error return can go

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm still actually a bit unhappy about this function.
Don't you think I should maybe remove the infinite loop from the Run method, and move the recurring logic part of the main file ?
So this package only holds logic about upgrade itself. And it's up to the caller to decide how often to run it ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure, the period is already decided by an argument. So the question is more, would someone ever want to not call all these actions together I guess?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah but if I remove the infinite loop, then I also remove the period argument.
And the Run method is actually the only think to run when you want to check/apply a new upgrade.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

True, I misunderstood your earlier comment, though you wanted to remove this function and just expose the individual functions that it calls. I think that is indeed acceptable.

@zaibon zaibon merged commit 26e637c into master Jun 14, 2019
@zaibon zaibon deleted the upgrade branch June 14, 2019 13:54
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.

4 participants