Skip to content
This repository has been archived by the owner on Jun 13, 2021. It is now read-only.

Warn user if the app bundle version differs with the current app version #769

Merged
merged 3 commits into from
Nov 29, 2019

Conversation

silvin-lubecki
Copy link
Contributor

- What I did
I now check current app version vs bundle app version if any and print a warning if it differs on the following commands:

  • inspect
  • pull
  • rm
  • run
  • update
  • image inspect
  • image render

- How I did it
I stored a new field "app-version" in the bundle's custom payload section, so I can compare built app image with the current app binary version.

- How to verify it

  • Build an app with this PR's binary
  • Edit the produced bundle directly in the bundle store (~/.docker/app/bundles/..../bundle.json), find where the custom payload is and change the app-version field
  • Inspect this application, the warning should be there

- Description for the changelog

  • Warn user if the app bundle version differs with the current app version

- A picture of a cute animal (not mandatory)
image

@codecov
Copy link

codecov bot commented Nov 27, 2019

Codecov Report

Merging #769 into master will decrease coverage by 1.56%.
The diff coverage is 55.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #769      +/-   ##
==========================================
- Coverage   72.09%   70.52%   -1.57%     
==========================================
  Files          67       67              
  Lines        3838     3729     -109     
==========================================
- Hits         2767     2630     -137     
- Misses        739      756      +17     
- Partials      332      343      +11
Impacted Files Coverage Δ
internal/commands/pull.go 64.28% <0%> (-4.95%) ⬇️
internal/commands/run.go 62.36% <0%> (-1.38%) ⬇️
internal/packager/cnab.go 96.98% <100%> (ø) ⬆️
internal/commands/image/inspect.go 66.66% <33.33%> (-2.73%) ⬇️
internal/commands/update.go 56.89% <33.33%> (-2.04%) ⬇️
internal/commands/inspect.go 48.38% <33.33%> (-1.62%) ⬇️
internal/commands/remove.go 50.94% <33.33%> (-2%) ⬇️
internal/commands/image/render.go 67.85% <52.38%> (-1.64%) ⬇️
internal/packager/custom.go 73.33% <87.5%> (+12.04%) ⬆️
internal/commands/build/build.go 60% <0%> (-20.42%) ⬇️
... and 1 more

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 e18898a...12e6785. Read the comment docs.

@silvin-lubecki silvin-lubecki marked this pull request as ready for review November 27, 2019 17:07
Copy link
Member

@eunomie eunomie left a comment

Choose a reason for hiding this comment

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

Looks good but one question about the test between versions and the message we are displaying

if versionPayload, ok := payload.(CustomPayloadAppVersion); ok {
version = versionPayload.AppVersion()
}
if version != internal.Version {
Copy link
Member

Choose a reason for hiding this comment

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

Here we are testing the versions are different, not the App Image has been built with a prior version of docker app. Maybe it's what we want, but I think the message should be different in that case, saying that App Image and docker app versions differ.

Copy link
Contributor

@zappy-shu zappy-shu left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
…rning if it differs on the following commands:

- inspect
- pull
- rm
- run
- update
- image inspect
- image render

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
@zappy-shu zappy-shu merged commit e244826 into docker:master Nov 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants