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

Sync pullspec to latest only when rpm for assembly is present in latest #1129

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

Conversation

Copy link
Contributor

openshift-ci bot commented Nov 15, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from thegreyd. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

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

@thegreyd thegreyd added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 15, 2024
Copy link
Contributor

openshift-ci bot commented Nov 18, 2024

@thegreyd: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 577c097 link false /test security

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

@@ -188,7 +188,8 @@ async def _trigger_microshift_sync(self):
major, minor = self._ocp_version
version = f'{major}.{minor}'
try:
jenkins.start_microshift_sync(version=version, assembly=self.assembly, dry_run=self.runtime.dry_run)
jenkins.start_microshift_sync(version=version, assembly=self.assembly,
dry_run=self.runtime.dry_run, block_until_complete=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

what dose the block_until_complete used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It blocks the parent job until this child job is complete

@@ -81,6 +82,10 @@ async def run(self):
data_path=self._doozer_env_vars["DOOZER_DATA_PATH"]
)
self.assembly_type = get_assembly_type(self.releases_config, self.assembly)

microshift_nvrs = await get_microshift_builds(self.group, self.assembly, env=self._elliott_env_vars)
Copy link
Contributor

Choose a reason for hiding this comment

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

for a named assembly, isn't the microshift nvr already pined in the assembly definition? why need to find builds again

Copy link
Contributor Author

@thegreyd thegreyd Nov 19, 2024

Choose a reason for hiding this comment

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

Yes, this func calls elliott which does the lookup from the assembly definition. It also validates it via brew.
This is the standard way of fetching it, like in build-microshift job

Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest we can directly load the nvr from assembly, the get_microshift_builds() is gong to invoke find-builds and the end result is the same as pined rpm.

# make sure that the latest path has the same microshift build as the given release assembly
# only then sync the pullspec to the latest path
latest_packages_path = f"{latest_path}/os/Packages/"
if await self.is_microshift_for_release_in_latest(latest_packages_path):
Copy link
Contributor

Choose a reason for hiding this comment

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

Will the microshift_bootc job triggered by microshift job? if its triggered by microshift job can this be a parameter of the job to update latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the build-microshift-bootc job is triggered by the build-microshift job.
So you're proposing that build-microshift job determine if latest needs update or not? And then sets the flag
Or are you proposing that someone set that flag by hand?
The enhancement here is to remove the manual part, i.e. we can do this basic check and only
update latest when rpms are sync'd first

Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of get the propose you block microshift-sync job above the wait it populates the path?
yeah, I think set a --latest arg will be simpler

@thegreyd thegreyd requested a review from Ximinhan November 19, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants