ostree icon indicating copy to clipboard operation
ostree copied to clipboard

Enhance command line utils to take a tree/file sha

Open wmanley opened this issue 7 years ago • 9 comments

...in place of a commit sha where reasonable.

TODO

  • [x] Remove debug prints
  • [x] Remove private API from symbol table
  • [x] Fix code formatting WRT space between function name and argument list
  • [x] Make CI pass
  • ~~Comment on "000000000000000000000000" SHA check~~ 000...000 removed. I replaced it with dirmeta_0755_0_0
  • [ ] Use g_file_query_info in place of g_file_query_file_type
  • [ ] Use g_assert (OSTREE_IS_REPO_FILE (root_gfile));
  • [x] Add more 0s to dummy SHA or replace it with something else. - replaced
  • [ ] Fix bug passing a single ref:filename argument to ostree diff

Description

The motivation for this is to enhance ostree commit --tree=ref to allow composing subtrees. For example: in our build system we strip away everything not under /usr as the last step when building containers. This PR (in combination with #1651) allows you to do:

ostree commit --tree=prefix=/usr/lib/machines/container/usr --tree=ref=:container:usr

To do this. It's fast.

As I was going along I thought it would be useful to have this ability generally in the command line tools. So now many of them accept a treeish instead of a commit.

Previously these tools accepted a COMMIT as:

  • REF
  • REMOTE:REF
  • COMMIT_SHA

And now in addition:

  • REMOTE:REF:PATH
  • :REF:PATH
  • COMMIT_SHA:PATH
  • TREE_SHA:PATH

So the idea is that you append :PATH to a commit and you can select a tree. This is the same as the git syntax. As a ref may already contain a colon if the REMOTE is specified we disambiguate by requiring the remote to be specified, even if it is left empty.

wmanley avatar Jun 28 '18 14:06 wmanley

Offhand the idea is cool, and I love to help push forward the container+OS as single tree concept and make that work better.

It's not a private API if we're adding it to the symbol table :smile:. We did add a special way for the CLI to call into private library bits, see ostree-cmdprivate.[ch]. That's probably a good approach until we decide to make the API stable?

In just glancing at the code: There are various leftover debug prints, and can you fix the space-between-identifer-and-parent i.e. g_strcmp0 ( vs g_strcmp0(.

Thanks again for working on this! Really looking forward to getting more of your patches in.

cgwalters avatar Jun 29 '18 13:06 cgwalters

:umbrella: The latest upstream changes (presumably c7b12a8) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot avatar Jul 09 '18 13:07 rh-atomic-bot

In just glancing at the code: There are various leftover debug prints, and can you fix the space-between-identifer-and-parent i.e. g_strcmp0 ( vs g_strcmp0(.

Sorry, that was sloppy. I was in a rush to finish it the week before last as I suspected that I wouldn't have time last week.

I've fixed the formatting issues - at least the ones I spotted. I had a crack at configuring GNU indent and clang-format to format according to ostree style, but failed on both. I couldn't get either to align the * in function definitions correctly. I think https://reviews.llvm.org/D27651 is the clang bug.

I've changed the approach to private API as recommended. We use ostree-cmdprivate.h now.

I've also broken up the main commit into 3 to make it easier to review. ~~CI passes too.~~

Please review.

wmanley avatar Jul 09 '18 17:07 wmanley

CI isn't passing, but I don't think it's related:

f28-rust says:

Error: Error downloading packages: Status code: 503 for https://mirrors.fedoraproject.org/metalink?repo=updates-released-f28&arch=x86_64

FAH28-insttests says:

Could not resolve host: kojipkgs.fedoraproject.org

wmanley avatar Jul 10 '18 10:07 wmanley

There's a bug in ostree diff introduced here. Running:

ostree --repo=/ostree ls :stbt-node-rootfs/b51ca22876b3b7ff776870bca1e8c1ff92e6bcbb:usr/bin

fails with

error: Bad ref ":stbt-node-rootfs/0b791b5379ea56d111307f112b2bce0cc5a2d732:usr/bin^": "bin^": No such file or directory

presumably because ostree diff by default adds ^ to the ref to get the previous ref. I'll need to add a test and fix this.

wmanley avatar Jul 17 '18 11:07 wmanley

:umbrella: The latest upstream changes (presumably 6869bad) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot avatar Jul 30 '18 17:07 rh-atomic-bot

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: wmanley To complete the pull request process, please assign jlebon You can assign the PR to them by writing /assign @jlebon in a comment when ready.

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

openshift-ci-robot avatar Jul 02 '20 10:07 openshift-ci-robot

@wmanley: 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-robot avatar Apr 12 '21 21:04 openshift-ci-robot

@wmanley: 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/sanity b4c51bceb14f402ea15be6a648ac5fbe04d3589b link true /test sanity

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.

openshift-ci[bot] avatar Apr 06 '22 20:04 openshift-ci[bot]