Skip to content

api: Change Platform field back to string (temporary workaround)#37381

Merged
tiborvass merged 1 commit into
moby:masterfrom
tiborvass:api-platforms-as-string-for-temp
Jul 4, 2018
Merged

api: Change Platform field back to string (temporary workaround)#37381
tiborvass merged 1 commit into
moby:masterfrom
tiborvass:api-platforms-as-string-for-temp

Conversation

@tiborvass
Copy link
Copy Markdown
Contributor

This partially reverts #37350

Although specs.Platform is desirable in the API, there is more work
to be done on helper functions, namely containerd's platforms.Parse
that assumes the default platform of the Go runtime.

That prevents a client to use the recommended Parse function to
retrieve a specs.Platform object.

With this change, no parsing is expected from the client.

Signed-off-by: Tibor Vass tibor@docker.com

@tiborvass
Copy link
Copy Markdown
Contributor Author

Ping @tonistiigi @dmcgowan @thaJeztah

Comment thread builder/builder-next/builder.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

empty lines

Comment thread builder/dockerfile/builder.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why the TrimSpace

Comment thread builder/dockerfile/internals.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just parse instead of HasPrefix.

Comment thread builder/builder-next/builder.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is never used

Comment thread builder/builder-next/builder.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not need to be formatted. Can be the original value.

@tonistiigi
Copy link
Copy Markdown
Member

Build errors in CI

@tiborvass tiborvass force-pushed the api-platforms-as-string-for-temp branch from 3bb1d7a to 8528237 Compare July 3, 2018 18:54
@tiborvass tiborvass requested a review from dnephin as a code owner July 3, 2018 18:54
Comment thread client/image_build.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

already set in imageBuildOptionsToQuery

Comment thread builder/dockerfile/internals.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

b.platform can be nil

Comment thread builder/dockerfile/builder.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: config.Platform != "" and other places

Comment thread api/server/router/build/build_routes.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Still TrimSpace in here for some reason.

@tiborvass tiborvass force-pushed the api-platforms-as-string-for-temp branch 2 times, most recently from 8ffda69 to 1a18380 Compare July 3, 2018 19:33
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 3, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@1da7d2e). Click here to learn what that means.
The diff coverage is 17.85%.

@@            Coverage Diff            @@
##             master   #37381   +/-   ##
=========================================
  Coverage          ?   34.93%           
=========================================
  Files             ?      610           
  Lines             ?    44880           
  Branches          ?        0           
=========================================
  Hits              ?    15678           
  Misses            ?    27084           
  Partials          ?     2118

@tiborvass tiborvass force-pushed the api-platforms-as-string-for-temp branch from 1a18380 to ae54ee7 Compare July 3, 2018 21:50
Comment thread builder/dockerfile/dispatchers_test.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As follow-up we should add a test that uses platform=goos and checks that it returns the original windows specific message.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread api/server/router/build/build_routes.go Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

should we keep the validation/error-handling as it was before we changed to this? https://github.com/moby/moby/pull/37350/files#diff-f0ccc740f5a5ed38075be6855298809cL77

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I see, you moved the validation to the build/next-builder.

@thaJeztah
Copy link
Copy Markdown
Member

@tiborvass needs a minor rebase

This partially reverts moby#37350

Although specs.Platform is desirable in the API, there is more work
to be done on helper functions, namely containerd's platforms.Parse
that assumes the default platform of the Go runtime.

That prevents a client to use the recommended Parse function to
retrieve a specs.Platform object.

With this change, no parsing is expected from the client.

Signed-off-by: Tibor Vass <tibor@docker.com>
@tiborvass tiborvass force-pushed the api-platforms-as-string-for-temp branch from ae54ee7 to facad55 Compare July 3, 2018 22:34
@tonistiigi
Copy link
Copy Markdown
Member

LGTM

1 similar comment
@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Jul 4, 2018

LGTM

@tiborvass
Copy link
Copy Markdown
Contributor Author

Failures on janky are flakey

@tiborvass tiborvass merged commit b711437 into moby:master Jul 4, 2018
@tiborvass tiborvass deleted the api-platforms-as-string-for-temp branch July 17, 2019 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants