Ask if a different version of a pkg should be installed when theres an existing installation #92

terev · 2019-08-19T05:43:40Z

This pr addresses a portion of #17 . It prompts the user to ask if they would like to overwrite their existing installation of a package with a new image. This was already possible via a forced installation but this wasn't evident to me at first at least.

codecov-io · 2019-08-19T05:46:49Z

Codecov Report

Merging #92 into master will decrease coverage by 7.44%.
The diff coverage is 30.86%.

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage    47.6%   40.16%   -7.45%     
==========================================
  Files          12       13       +1     
  Lines         439      717     +278     
==========================================
+ Hits          209      288      +79     
- Misses        211      405     +194     
- Partials       19       24       +5

Impacted Files	Coverage Δ
cmd/install.go	`8.33% <0%> (-1.84%)`	⬇️
packages/manager.go	`65.82% <0%> (-4.45%)`	⬇️
packages/package.go	`61.9% <34.78%> (-11.96%)`	⬇️
packages/package_diff.go	`34.84% <34.84%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fca86f8...b35e956. Read the comment docs.

tjamet

Please update the messages and try to provide the list of changes to be added/removed when updating a package

packages/package.go

cmd/install.go

tjamet · 2019-08-21T12:56:55Z

Thanks for your contribution though, this feature is pretty nice, just need a bit more polishing to be amazing!

terev · 2019-08-24T01:01:25Z

@tjamet I've made some changes and have replied with questions I could use your opinion about. I have another feature branched off of this one that I'd like to submit if we get this merged.

In case you're curious, the other feature is an option to automatically detect the path to an executable file in the given image and set the entrypoint. This seems like a nice feature so that public images can be used more easily. I've been using it to switch versions of golang and ruby locally when i need to. When developing this I referred to #20 for ideas. https://github.com/terev/whalebrew/tree/detect-entrypoint

terev · 2019-08-29T04:52:37Z

@bfirsh looking for another review on this one if someone has time

tjamet

Thanks for the contribution
There are some nits we can address here or in a different PR

cmd/install.go

packages/package.go

terev · 2019-09-02T22:20:42Z

@tjamet is there a discord group or something where we could talk about the project? I have a few questions

tjamet · 2019-09-03T09:44:45Z

there is no discord group so far, shall you send the questions by email?

terev · 2019-09-09T06:28:35Z

@tjamet I've modified the output provided when there's changes to an existing installation of a package. I've also added the functionality to only ask the user about permission changes when installing a different image for a package. Sure that would be good my email is on my profile.

terev · 2019-09-10T06:14:00Z

Some sample output:

./whalebrew install cmd:v1.0.1
Looks like you already have cmd:test installed as /usr/local/bin/cmd.
Would you like to change cmd:test to cmd:v1.0.1? (y/n) [y]: y
Updating this package will remove some of its current permissions:

* Read the environment variable W
* Read the file or directory "a"

Updating this package will modify some of its current permissions

* Read changed to read and write of the file or directory "~/.aws"

Is this okay? (y/n) [y]: y
🐳  Modified /usr/local/bin/cmd to use cmd:v1.0.1

…existing installation

…n modifying image

…it to yml configs

… conditional

…all of new image for package

tjamet

One concern, but can be solved independently
merging

tjamet · 2019-09-13T09:25:31Z

cmd/install.go

+				if changed, diff, err := installed.HasChanges(ctx, cli); err != nil {
+					return err
+				} else if changed {
+					fmt.Println("There are differences between the installed version of the package and the image:")


I would prefer a stated message like

Updating this package would be granted different permissions
Added access to environment variables:
<...>

terev · 2019-09-14T05:43:22Z

@tjamet Thank you so much for having a look at these changes! I really appreciate this project and the potential it has to be really useful. Thank you for maintaining it and being part of it's success.

I'm going to be working on a new pr soon that will allow installing public images without the need for a predefined entrypoint. This idea is along the lines of what is detailed here #20 . This will enable things like easily running arbitrary versions of interpreters or even golang :)

terev mentioned this pull request Aug 20, 2019

Allow installation of a specific version #17

Open

3 tasks

tjamet requested changes Aug 21, 2019

View reviewed changes

tjamet approved these changes Aug 29, 2019

View reviewed changes

cmd/install.go Outdated Show resolved Hide resolved

packages/package.go Outdated Show resolved Hide resolved

packages/package.go Outdated Show resolved Hide resolved

terev requested a review from tjamet September 9, 2019 06:28

terev force-pushed the master branch 2 times, most recently from b329cac to 8b6f302 Compare September 10, 2019 06:24

terev added 11 commits September 10, 2019 02:33

ask if a different version of pkg should be installed when theres an …

9fe0c59

…existing installation

obey forceinstall

eca0e8f

fix var naming standard

4b1557a

added check to avoid unknowingly overwriting changes to a package whe…

7a7717c

…n modifying image

add module downloads as separate step, add retry

1b63e52

maintain the default working dir programmatically rather than adding …

932d800

…it to yml configs

dont error if there's no changes to be made

c24cc96

print the changes that would be overwritten in a diff format

fbfb6a5

modified message to say the package modified, and removed unnecessary…

d2cd9fa

… conditional

added functionality to only ask user about permission changes on inst…

92fc070

…all of new image for package

add missing word

b35e956

terev force-pushed the master branch from 8b6f302 to b35e956 Compare September 10, 2019 06:35

tjamet approved these changes Sep 13, 2019

View reviewed changes

tjamet merged commit c3ac8df into whalebrew:master Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ask if a different version of a pkg should be installed when theres an existing installation #92

Ask if a different version of a pkg should be installed when theres an existing installation #92

terev commented Aug 19, 2019

codecov-io commented Aug 19, 2019 •

edited

tjamet left a comment

tjamet commented Aug 21, 2019

terev commented Aug 24, 2019 •

edited

terev commented Aug 29, 2019

tjamet left a comment

terev commented Sep 2, 2019

tjamet commented Sep 3, 2019

terev commented Sep 9, 2019

terev commented Sep 10, 2019 •

edited

tjamet left a comment

tjamet Sep 13, 2019

terev commented Sep 14, 2019 •

edited

Ask if a different version of a pkg should be installed when theres an existing installation #92

Ask if a different version of a pkg should be installed when theres an existing installation #92

Conversation

terev commented Aug 19, 2019

codecov-io commented Aug 19, 2019 • edited

Codecov Report

tjamet left a comment

Choose a reason for hiding this comment

tjamet commented Aug 21, 2019

terev commented Aug 24, 2019 • edited

terev commented Aug 29, 2019

tjamet left a comment

Choose a reason for hiding this comment

terev commented Sep 2, 2019

tjamet commented Sep 3, 2019

terev commented Sep 9, 2019

terev commented Sep 10, 2019 • edited

tjamet left a comment

Choose a reason for hiding this comment

tjamet Sep 13, 2019

Choose a reason for hiding this comment

terev commented Sep 14, 2019 • edited

codecov-io commented Aug 19, 2019 •

edited

terev commented Aug 24, 2019 •

edited

terev commented Sep 10, 2019 •

edited

terev commented Sep 14, 2019 •

edited