Skip to content

fix: s2i build as-dockerfile mode#1063

Merged
knative-prow[bot] merged 1 commit intoknative:mainfrom
matejvasek:as-dockerfile-s2i
Jun 15, 2022
Merged

fix: s2i build as-dockerfile mode#1063
knative-prow[bot] merged 1 commit intoknative:mainfrom
matejvasek:as-dockerfile-s2i

Conversation

@matejvasek
Copy link
Copy Markdown
Contributor

@matejvasek matejvasek commented Jun 14, 2022 •
edited
Loading

Changes

  • Use "as dockerfile" mode of s2i builds:
    Rationale: podman < 4.0 has faulty /commit endpoint so "daemon mode" is not working

/kind bug

fix: s2i+podman<4.0 build error "CommitFailure: invalid change "LABEL" - must be formatted as KEY VALU"

@knative-prow knative-prow bot added do-not-merge/work-in-progress 🤖 PR should not merge because it is a work in progress. kind/bug Bugs size/L 🤖 PR changes 100-499 lines, ignoring generated files. approved 🤖 PR has been approved by an approver from all required OWNERS files. labels Jun 14, 2022
@matejvasek
Copy link
Copy Markdown
Contributor Author

matejvasek commented Jun 14, 2022

This version inspects appropriate label and sets s2i script url. This fixes the issue with openjdk builds.

@codecov
Copy link
Copy Markdown

codecov bot commented Jun 14, 2022 •
edited
Loading

Codecov Report

Merging #1063 (f525ab4) into main (5f6d11c) will increase coverage by 0.45%.
The diff coverage is 57.14%.

❗ Current head f525ab4 differs from pull request most recent head c57c502. Consider uploading reports for the commit c57c502 to get more accurate results

@@            Coverage Diff             @@
##             main    #1063      +/-   ##
==========================================
+ Coverage   46.79%   47.24%   +0.45%     
==========================================
  Files          59       58       -1     
  Lines        7761     7854      +93     
==========================================
+ Hits         3632     3711      +79     
+ Misses       3783     3782       -1     
- Partials      346      361      +15
Impacted Files Coverage Δ
s2i/builder.go 65.74% <57.14%> (-5.05%) ⬇️

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 5f6d11c...c57c502. Read the comment docs.

@knative-prow knative-prow bot added size/XL 🤖 PR changes 500-999 lines, ignoring generated files. and removed size/L 🤖 PR changes 100-499 lines, ignoring generated files. labels Jun 14, 2022
@matejvasek matejvasek force-pushed the as-dockerfile-s2i branch 2 times, most recently from 834c3e0 to 086245f Compare June 14, 2022 18:10
@matejvasek
Use "as-dockerfile" mode for s2i builds
c57c502 
The "daemon mode" doesn't work well with `podman`.

Signed-off-by: Matej Vasek <mvasek@redhat.com>
@jrangelramos
Copy link
Copy Markdown
Contributor

jrangelramos commented Jun 14, 2022

recent changes tested w/ podman

@matejvasek matejvasek marked this pull request as ready for review June 14, 2022 18:43
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress 🤖 PR should not merge because it is a work in progress. label Jun 14, 2022
@matejvasek
Copy link
Copy Markdown
Contributor Author

matejvasek commented Jun 14, 2022

@lance @lkingland @zroubalik PTAL

@matejvasek
Copy link
Copy Markdown
Contributor Author

matejvasek commented Jun 14, 2022

Too bad this won't make it to midstream release. Maybe we could do at least upstream release?

zroubalik
zroubalik approved these changes Jun 15, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

/lgtm

/hold - merge as you like

@knative-prow knative-prow bot added the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jun 15, 2022
@knative-prow knative-prow bot added the lgtm 🤖 PR is ready to be merged. label Jun 15, 2022
@knative-prow
Copy link
Copy Markdown

knative-prow bot commented Jun 15, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matejvasek, zroubalik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [matejvasek,zroubalik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@lkingland
Copy link
Copy Markdown
Member

lkingland commented Jun 15, 2022

Good workaround
/lgtm as well

@matejvasek
Copy link
Copy Markdown
Contributor Author

matejvasek commented Jun 15, 2022

To be honest I not super happy about this since this introduces unnecessary project copy but this is only way how to make this work on podman v3.3.

@matejvasek
Copy link
Copy Markdown
Contributor Author

matejvasek commented Jun 15, 2022

On the bright side this may make "cross-platform" build with multi-arch images (s2i is mulitarch) easier.

@lance lance changed the title s2i build as-dockerfile mode fix: s2i build as-dockerfile mode Jun 15, 2022
@lance
Copy link
Copy Markdown
Contributor

lance commented Jun 15, 2022

/lgtm
/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold 🤖 PR should not merge because someone has issued a /hold command. label Jun 15, 2022
@knative-prow knative-prow bot merged commit 2721ae6 into knative:main Jun 15, 2022
@matejvasek matejvasek mentioned this pull request Apr 2, 2025
Merged
gauron99 added a commit to gauron99/func that referenced this pull request May 6, 2025
@gauron99
add-gauron (knative#1063)
8092660 
Signed-off-by: David Fridrich <fridrich.david19@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lkingland lkingland Awaiting requested review from lkingland

@lance lance Awaiting requested review from lance

@jrangelramos jrangelramos Awaiting requested review from jrangelramos

1 more reviewer

@zroubalik zroubalik zroubalik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@lance lance

@zroubalik zroubalik

Labels

approved 🤖 PR has been approved by an approver from all required OWNERS files. kind/bug Bugs lgtm 🤖 PR is ready to be merged. size/XL 🤖 PR changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@matejvasek @jrangelramos @lkingland @lance @zroubalik