Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

features: installer artifacts build paralleled, do not build up-to-date images #39

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

dsevost
Copy link
Contributor

@dsevost dsevost commented Jun 7, 2023

hello, dear okd-team,
please review the pull-req

features added:

  • long-running installer artifacts build process is splitted on separated sub-builds for particular arch and os, and then aggregated on next pipeline task
  • build outdated only images also dramatically speed-up build time

update: also included okd-rpms content places into release:artifacts

@aleskandro aleskandro self-assigned this Jul 7, 2023
Copy link
Member

@aleskandro aleskandro left a comment

Choose a reason for hiding this comment

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

Hi @dsevost , I apologize again for the substantial delay. The code overall is good. However, I'm unsure if we can deviate from the Prow Builds definitions today and successfully maintain this repo.

I would make two/three PRs from this large one:

  1. General fixes (for builder-replacement, buildArgs, and other kustomization patches, and to update fedora ISTags)
  2. The new feature to avoid rebuilding from scratch everytime and only build the outdated projects
  3. Changes to the way the installer-artifacts are built (but, as anticipated, I'm not sure this is ok to be done now, I'd also ask an opinion from @vrutkovs).

type: Local
importPolicy:
importMode: PreserveOriginal # Needs OKD 4.13
# upstream already resides on fedora:37 (Dockerfile.okd -> Dockerfile.fcos)
Copy link
Member

Choose a reason for hiding this comment

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

I'd add the fedora:3[78] ISTags.

@@ -57,6 +56,7 @@ spec:
value: >
OpenShift is a platform for developing,
building, and deploying containerized applications.
# are these tag really necessary here?
Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly these are replaced later with the okd ones and are needed for some specific code to be considered for OKD vs OCP vs OKD/SCOS. I'm not sure if this is needed for the cli too though, @vrutkovs ?

@@ -25,6 +25,7 @@ spec:
imageOptimizationPolicy: SkipLayers
dockerfilePath: images/baremetal/Dockerfile.centos9
buildArgs:
# are these tag really necessary here?
Copy link
Member

Choose a reason for hiding this comment

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

Yes

metadata:
name: installer-artifact-amd64-lnx
labels:
part-of-artifacts: installer
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is ideal at this time. Deviating from the prow builds definitions may be a future step. However, the maintenance of the repo might become harder including these changes..

name: 'tools:fedora36'
as:
- 'fedora:35'
# - from:
Copy link
Member

Choose a reason for hiding this comment

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

Either delete or update, I'd avoid commenting. I'd prefer updating as per the previous comment.

@@ -10,31 +11,103 @@ spec:
- name: max_concurrent_builds
type: string
default: "8"
- name: build_outdated_only
Copy link
Member

Choose a reason for hiding this comment

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

Can we get the changes regarding this feature in a separate PR?

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