Skip to content

ZEA-5097: allow specifying the dockerfile path#465

Merged
pan93412 merged 12 commits into
mainfrom
pan93412/zea-5097-allow-specifying-the-dockerfile-path
May 17, 2025
Merged

ZEA-5097: allow specifying the dockerfile path#465
pan93412 merged 12 commits into
mainfrom
pan93412/zea-5097-allow-specifying-the-dockerfile-path

Conversation

@pan93412
Copy link
Copy Markdown
Member

@pan93412 pan93412 commented May 17, 2025

Description (required)

  • Implement PlanV2 and PackerV2 to support advanced Match()
  • Implement dockerfile.path to specify an explicit path (like `docker/frontend.Dockerfile)
  • Remove spamming error message (like empty version)
  • Add tests to cover this part.

Related issues & labels (optional)

  • Closes ZEA-5097
  • Suggested label: bug

@pan93412 pan93412 self-assigned this May 17, 2025
@pan93412 pan93412 force-pushed the pan93412/zea-5097-allow-specifying-the-dockerfile-path branch from 5a79163 to 2311de7 Compare May 17, 2025 05:07
@pan93412 pan93412 requested a review from Copilot May 17, 2025 05:07
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR enhances the Dockerfile support by migrating identifiers/packers to V2 interfaces, adding a configurable dockerfile.path option, cleaning up noisy errors, and extending test coverage for both dockerfile.name and dockerfile.path.

  • Migrate existing Identifier and Packer implementations to new IdentifierV2 and V2 interfaces via wrapper functions.
  • Introduce dockerfile.path in the schema and code to allow explicit Dockerfile file paths.
  • Remove redundant version error logs and add ErrEmptyVersion; update relevant tests and schema.

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
schema/zbpack.json Added dockerfile.path schema, expanded descriptions/examples.
pkg/zeaburpack/packers.go Converted SupportedPackers to return V2 packers, wrapping old packers.
pkg/zeaburpack/identifiers.go Converted SupportedIdentifiers to return V2 identifiers.
internal/utils/version.go Introduced ErrEmptyVersion, replaced error string with sentinel error.
internal/dockerfile/finder.go New FindDockerfile implementing path/name resolution.
internal/dockerfile/plan.go Refactored ReadDockerfile/GetMeta to use new finder and error wraps.
internal/dockerfile/identify.go Updated Match to use FindDockerfile and handle NotExist vs other errors.
Comments suppressed due to low confidence (1)

pkg/zeaburpack/packers.go:28

  • [nitpick] To align with the wrapping style used for other packers (e.g., packer.WrapV2(dart.NewPacker())), consider wrapping dockerfile.NewPacker() with packer.WrapV2 or document that NewPacker already returns a V2 implementation.
dockerfile.NewPacker(),

Comment thread internal/dockerfile/finder.go Outdated
Comment thread schema/zbpack.json Outdated
Comment thread pkg/zeaburpack/identifiers.go
@pan93412 pan93412 requested a review from Copilot May 17, 2025 05:16
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR upgrades the packer/identifier APIs to V2, adds support for explicitly specifying a Dockerfile path via dockerfile.path, refines version parsing to suppress empty-version spam, and adds corresponding tests.

  • Introduce V2 interfaces for both packers and identifiers and wrap existing implementations.
  • Implement dockerfile.path lookup in internal/dockerfile/finder.go and update discovery logic.
  • Define ErrEmptyVersion and adjust version parsing/logging to avoid spamming on empty input.

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/dockerfile-test/path/zbpack.json Add test config for explicit Dockerfile path
schema/zbpack.json Document dockerfile.path and update name description
pkg/zeaburpack/packers.go Change SupportedPackers to return V2 and wrap packers
pkg/zeaburpack/identifiers.go Change SupportedIdentifiers to return IdentifierV2
pkg/plan/plan.go Migrate Planner to use IdentifierV2
internal/utils/version.go Introduce ErrEmptyVersion and refine error handling
internal/dockerfile/finder.go New FindDockerfile with dockerfile.path support
internal/dockerfile/identify.go Update identifier to use new finder and V2 interface
internal/dockerfile/plan.go Refactor ReadDockerfile/Meta to use new finder context
internal/dockerfile/dockerfile.go Update packer to implement V2 interface
Comments suppressed due to low confidence (2)

pkg/zeaburpack/packers.go:22

  • [nitpick] The comment refers to identifiers but this function returns packers; update it to "SupportedPackers returns all supported packers."
// SupportedPackers returns all supported identifiers

internal/utils/version.go:122

  • [nitpick] Include the original constraint string in the log for better context, e.g., log.Printf("invalid version %q: %v", constraints, err).
log.Println("invalid version", err)

Comment thread internal/dockerfile/finder.go Outdated
Comment thread internal/dockerfile/finder.go
@pan93412 pan93412 force-pushed the pan93412/zea-5097-allow-specifying-the-dockerfile-path branch from 376c26c to 3bdecfa Compare May 17, 2025 05:19
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@pan93412 pan93412 requested a review from Copilot May 17, 2025 05:21
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Implements advanced Dockerfile resolution and migrates packer/identifier APIs to V2.

  • Introduce dockerfile.path in schema and tests, allowing explicit Dockerfile paths.
  • Refactor packer/identifier layers to V2 interfaces, wrapping existing implementations.
  • Consolidate Dockerfile find/read logic into a new module, and suppress noisy “empty version” logs in version parsing.

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/dockerfile-test/path/zbpack.json Add test config using dockerfile.path
tests/dockerfile-test/path/docker/test.Dockerfile Sample Dockerfile for path-based tests
schema/zbpack.json Add path property and update name description
pkg/zeaburpack/packers.go Switch SupportedPackers to return packer.V2, wrap old types
pkg/zeaburpack/identifiers.go Switch SupportedIdentifiers to return plan.IdentifierV2
internal/dockerfile/finder.go New FindDockerfile logic handling dockerfile.path/name
internal/dockerfile/dockerfile.go Update Dockerfile packer to implement V2 interface
internal/utils/version.go Introduce ErrEmptyVersion and remove spammy error logs
Comments suppressed due to low confidence (3)

internal/dockerfile/dockerfile.go:17

  • The comment for NewPacker incorrectly mentions a Deno packer; update it to reference the Dockerfile packer.
// NewPacker returns a new Deno packer.

schema/zbpack.json:124

  • [nitpick] The examples for dockerfile.path include leading slashes that imply absolute paths; clarify in the description whether paths are always relative to the project root.
"examples": ["Dockerfile", "/Dockerfile", "/docker/custom.Dockerfile"]

pkg/zeaburpack/identifiers.go:23

  • Update the comment to note that this now returns []plan.IdentifierV2 rather than the old []plan.Identifier to match the new API signature.
// SupportedIdentifiers returns all supported identifiers

Comment thread internal/dockerfile/identify.go
@pan93412 pan93412 merged commit c9f5f8e into main May 17, 2025
5 checks passed
@pan93412 pan93412 deleted the pan93412/zea-5097-allow-specifying-the-dockerfile-path branch May 17, 2025 05:24
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.

2 participants