graph-builder: switch to using dkregistry and implement authenticated requests #4

steveej · 2018-10-12T17:23:30Z

No description provided.

graph-builder/Cargo.toml

docs/design/cincinnati.md

graph-builder/src/registry.rs

graph-builder/src/main.rs

graph-builder/Cargo.toml

graph-builder/src/registry.rs

crawford · 2018-11-02T16:26:14Z

graph-builder/src/registry.rs

+        .trim_left_matches("http://")
+}
+
+fn get_credentials_or_empty_reader(credentials_path: &Option<PathBuf>) -> Box<dyn std::io::Read> {


If you just inline this function, you can avoid the heap allocation. I don't think this helps much with readability.

Using Box<dyn std::io::Read> as the return type is the only way I've found to tell the compiler that all cases yield a reader. I've double-checked inlining this yesterday and couldn't make it work. Do you want me to triple-check?

I managed to inline the functionality yet the Box is still required. I suggest we leave it in and revisit on subsequent compiler releases.

graph-builder/src/registry.rs

crawford

I haven't made it all the way through this yet. clippy also has some suggestions you should follow.

graph-builder/src/registry.rs

crawford · 2018-11-12T23:57:32Z

graph-builder/src/registry.rs

+    ) {
+        Ok(mut tags) => {
+            if tags.len() == 0 {
+                bail!("{}/{} has no tags", registry, repo);


This doesn't seem like an error condition.

Please consider this in the answer to https://github.com/openshift/cincinnati/pull/4/files/835a3685c6c29f76bb999b96ec72f4ce00b37550#r233220468

crawford · 2018-11-12T23:57:55Z

graph-builder/src/registry.rs

+            if tags.len() == 0 {
+                bail!("{}/{} has no tags", registry, repo);
+            }
+            tags.sort();


Why bother sorting the list?

I designed the tests in a way to expect a certain vector of results because the documentation says the tags are returned in a lexical sorted order. Quay seems to diverge from that so this sort enforces what the docker registry API documents.

We should really try to avoid coupling to Quay as much as possible. CI cannot depend on it being up and they have already expressed concern over the amount of load we are putting on them. I'd rather we just adjusted our tests instead of the product.

It's possible to neglect the order in the tests, with the downside of making a failed test more difficult to debug.

graph-builder/src/registry.rs

crawford · 2018-11-13T00:21:17Z

graph-builder/src/registry.rs

+        .and_then(|tag_layer_digests| match tag_layer_digests {
+            Ok((tag, layer_digests)) => {
+                if layer_digests.len() == 0 {
+                    bail!("layer_digests list empty")


This doesn't seem like an error condition. We can handle this gracefully.

Setting the context for the discussion: this is the case when there are no layers available for a discovered tag, which seems highly unexpected. Do you think it's better to just log the errors and drive the whole chain to completion and yield an empty result vector?

Yes. Logging the warning and then just returning an empty vector is fine.

I haven't tested this behavior yet. It's not possible to upload an image without layers so this would include a bit of mocking.

Using the opportunity to update all dependencies.

rustfmt 0.99.6-nightly (750b252 2018-10-18)

… calls Switch to using dkregistry for all registry requests which supports asynchronous authenticated calls towards image registries. This commit also adds the parameter `--credentials-file` which takes a path to a JSON file as produced by `docker login`[1]. [1]: https://docs.docker.com/engine/reference/commandline/login/

crawford · 2018-11-16T20:48:36Z

/lgtm

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 12, 2018

steveej changed the title ~~[WIP] graph-builder: switch to using dkregistry and implement support authenticated requests~~ [WIP] graph-builder: switch to using dkregistry and implement authenticated requests Oct 12, 2018

steveej commented Oct 12, 2018

View reviewed changes

graph-builder/Cargo.toml Outdated Show resolved Hide resolved

docs/design/cincinnati.md Outdated Show resolved Hide resolved

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

steveej changed the title ~~[WIP] graph-builder: switch to using dkregistry and implement authenticated requests~~ [WIP][TOO EARLY TO REAVIEW] graph-builder: switch to using dkregistry and implement authenticated requests Oct 12, 2018

steveej changed the title ~~[WIP][TOO EARLY TO REAVIEW] graph-builder: switch to using dkregistry and implement authenticated requests~~ [WIP][TOO EARLY TO REVIEW] graph-builder: switch to using dkregistry and implement authenticated requests Oct 12, 2018

steveej force-pushed the switch-to-dkregistry branch 2 times, most recently from 28ff3a3 to 44a9c2a Compare October 12, 2018 22:35

steveej changed the title ~~[WIP][TOO EARLY TO REVIEW] graph-builder: switch to using dkregistry and implement authenticated requests~~ [WIP] graph-builder: switch to using dkregistry and implement authenticated requests Oct 12, 2018

steveej force-pushed the switch-to-dkregistry branch 3 times, most recently from 1b8ad70 to f983bff Compare October 12, 2018 22:44

crawford suggested changes Oct 16, 2018

View reviewed changes

steveej mentioned this pull request Oct 17, 2018

API: substitute Result<Future..> with Future.. for more idiomatic future chaining camallo/dkregistry-rs#49

Merged

7 tasks

steveej force-pushed the switch-to-dkregistry branch 5 times, most recently from b6b254e to 103816b Compare October 27, 2018 21:10

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 27, 2018

steveej force-pushed the switch-to-dkregistry branch 2 times, most recently from 9c8e925 to aab6d10 Compare October 27, 2018 22:52

crawford reviewed Oct 30, 2018

View reviewed changes

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

steveej force-pushed the switch-to-dkregistry branch from aab6d10 to dd96805 Compare October 31, 2018 21:06

steveej changed the title ~~[WIP] graph-builder: switch to using dkregistry and implement authenticated requests~~ graph-builder: switch to using dkregistry and implement authenticated requests Oct 31, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2018

crawford suggested changes Oct 31, 2018

View reviewed changes

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

graph-builder/src/registry.rs Outdated Show resolved Hide resolved

steveej force-pushed the switch-to-dkregistry branch from accebb5 to e87459b Compare November 1, 2018 09:23

steveej mentioned this pull request Nov 2, 2018

Add tests for fetch_releases pulling test images from quay.io #8

Closed

crawford suggested changes Nov 2, 2018

View reviewed changes

steveej force-pushed the switch-to-dkregistry branch from 3fc21d9 to dec0bc6 Compare November 6, 2018 17:01

steveej mentioned this pull request Nov 8, 2018

[deprecated] graph-builder: support image manifest labels #9

Closed

openshift-bot added the needs-rebase label Nov 9, 2018

steveej force-pushed the switch-to-dkregistry branch from cf736fc to 1c8c844 Compare November 12, 2018 22:03

openshift-bot removed the needs-rebase label Nov 12, 2018

steveej force-pushed the switch-to-dkregistry branch from 1c8c844 to 835a368 Compare November 12, 2018 23:15

steveej mentioned this pull request Nov 12, 2018

add tests for fetch_releases pulling test images from quay.io #11

Merged

crawford suggested changes Nov 13, 2018

View reviewed changes

steveej force-pushed the switch-to-dkregistry branch 5 times, most recently from 9924ec2 to ac7ac6f Compare November 16, 2018 17:15

cargo: update all dependencies

e259624

Using the opportunity to update all dependencies.

steveej force-pushed the switch-to-dkregistry branch 2 times, most recently from 7a80125 to a594e53 Compare November 16, 2018 20:44

steveej added 3 commits November 16, 2018 21:45

cargo: add dkregistry

6db549f

*: cargo +nightly fmt --all

d4524d5

rustfmt 0.99.6-nightly (750b252 2018-10-18)

steveej force-pushed the switch-to-dkregistry branch from a594e53 to b074a25 Compare November 16, 2018 20:47

crawford approved these changes Nov 16, 2018

View reviewed changes

openshift-ci-robot assigned crawford Nov 16, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2018

crawford merged commit 2e37a45 into openshift:master Nov 16, 2018

steveej deleted the switch-to-dkregistry branch November 17, 2018 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graph-builder: switch to using dkregistry and implement authenticated requests #4

graph-builder: switch to using dkregistry and implement authenticated requests #4

steveej commented Oct 12, 2018

crawford Nov 2, 2018

steveej Nov 2, 2018

steveej Nov 6, 2018

crawford left a comment

crawford Nov 12, 2018

steveej Nov 13, 2018

crawford Nov 12, 2018

steveej Nov 13, 2018

crawford Nov 13, 2018

steveej Nov 14, 2018

crawford Nov 13, 2018

steveej Nov 13, 2018 •

edited

Loading

crawford Nov 13, 2018

steveej Nov 14, 2018

crawford commented Nov 16, 2018

graph-builder: switch to using dkregistry and implement authenticated requests #4

graph-builder: switch to using dkregistry and implement authenticated requests #4

Conversation

steveej commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crawford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveej Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crawford commented Nov 16, 2018

steveej Nov 13, 2018 •

edited

Loading