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

composetree: allow url in manifest package list #1847

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mike-nguyen
Copy link
Member

This allows URLs to be used in the manifest file when doing a
treecompose only in unified-core mode. This is a convenient way of
adding one off packages for testing purposes but should not be the
preferred method of composing a tree.

The import local rpm functions were also moved to rpmostree-importer
to facilitate usage.

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 79dfcea) made this pull request unmergeable. Please resolve the merge conflicts.

@jlebon
Copy link
Member

jlebon commented Jun 4, 2019

Ahh looks like #1745 conflicted. Mind rebasing?

This allows URLs to be used in the manifest file when doing a
treecompose only in unified-core mode. This is a convenient way of
adding one off packages for testing purposes but should not be the
preferred method of composing a tree.

The import local rpm functions were also moved to rpmostree-importer
to facilitate usage.
@mike-nguyen
Copy link
Member Author

Rebased

}

gboolean
rpmostree_importer_import_many_local_rpms (OstreeRepo *repo,
Copy link
Member

Choose a reason for hiding this comment

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

I think given that this in in the importer now, maybe just rpmostree_importer_import_rpms ?

Copy link
Member

Choose a reason for hiding this comment

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

Let's also make this code move a separate prep commit? I think that'll clean up the history nicely make it easier to review the critical changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think given that this in in the importer now, maybe just rpmostree_importer_import_rpms ?

Sounds good. I was trying to think of a better but I am terrible with naming.

Let's also make this code move a separate prep commit? I think that'll clean up the history nicely make it easier to review the critical changes.

Will do.

/* We download URI packages and import them into the pkgcache first so we can
add it to the local packages to install */
g_autoptr(GUnixFDList) fd_list = g_unix_fd_list_new ();
for (char **it = pkgnames; it && *it; it++)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, were you able to try out the cached-packages approach we discussed? Should mostly be about taking this diff hunk here and putting it on the compose side, and then feeding the NEVRAs that the importer returns into the cached-packages field.

Copy link
Member Author

@mike-nguyen mike-nguyen Jun 10, 2019

Choose a reason for hiding this comment

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

Hmm, were you able to try out the cached-packages approach we discussed? Should mostly be about taking this diff hunk here and putting it on the compose side,

@jlebon Can you clarify on what you mean by putting it on the compose side?

and then feeding the NEVRAs that the importer returns into the cached-packages field.

Should I create another cached-packages array and add the NEVRAS in the loop at https://github.com/projectatomic/rpm-ostree/pull/1847/files#diff-07963b1a5922690400970963652cbe3bR1967 ?

@openshift-ci-robot
Copy link
Collaborator

@mike-nguyen: PR needs rebase.

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/test-infra repository.

@openshift-ci
Copy link

openshift-ci bot commented Jun 6, 2023

@mike-nguyen: The following tests 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/build bee767b link /test build
ci/prow/unit bee767b link /test unit
ci/prow/clang-analyzer bee767b link true /test clang-analyzer
ci/prow/build-clang bee767b link true /test build-clang
ci/prow/images bee767b link true /test images
ci/prow/fcos-e2e bee767b link true /test fcos-e2e
ci/prow/kola-upgrade bee767b link true /test kola-upgrade

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/test-infra repository. I understand the commands that are listed here.

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.

4 participants