Implement image spec #25

Furisto · 2021-08-19T20:23:38Z

Solves #10. Another PR with additional tests will follow.

codecov-commenter · 2021-08-19T20:31:39Z

Codecov Report

Merging #25 (8cf7e46) into main (1b42768) will decrease coverage by 0.84%.
The diff coverage is 16.56%.

@@            Coverage Diff             @@
##             main      #25      +/-   ##
==========================================
- Coverage   20.31%   19.46%   -0.85%     
==========================================
  Files           7       11       +4     
  Lines         699      863     +164     
  Branches      371      463      +92     
==========================================
+ Hits          142      168      +26     
- Misses        274      354      +80     
- Partials      283      341      +58

saschagrunert

Looking good, I just have a few nits.

saschagrunert · 2021-08-20T07:32:08Z

src/image/config.rs

+use serde::ser::SerializeMap;
+use serde::{Deserialize, Deserializer, Serialize, Serializer};


nit, unfortunately merge_imports is still an unstable feature of rustfmt:

Suggested change

use serde::ser::SerializeMap;

use serde::{Deserialize, Deserializer, Serialize, Serializer};

use serde::{Deserialize, Deserializer, Serialize, Serializer, ser::SerializeMap};

saschagrunert · 2021-08-20T07:32:31Z

src/image/config.rs

+);
+
+impl ImageConfiguration {
+    pub fn load<P: AsRef<Path>>(path: P) -> Result<ImageConfiguration> {


Let's add a doc comment.

saschagrunert · 2021-08-20T07:34:11Z

src/image/config.rs

+        /// a filesystem diff. It is set to true if this history item
+        /// doesn't correspond to an actual layer in the rootfs section
+        #[serde(skip_serializing_if = "Option::is_none")]
+        empty_layer: Option<bool>,


Suggested change

empty_layer: Option<bool>,

#[cfg_attr(feature = "builder", getset(get_copy = "pub"))]

empty_layer: Option<bool>,

saschagrunert · 2021-08-20T07:34:35Z

src/image/config.rs

+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),
+        builder(default, pattern = "owned", setter(into, strip_option)),
+        getset(get = "pub")


Suggested change

getset(get = "pub")

saschagrunert · 2021-08-20T07:34:49Z

src/image/config.rs

+    #[derive(Clone, Debug, Default, Deserialize, Eq, PartialEq, Serialize)]
+    #[cfg_attr(
+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),


Suggested change

derive(derive_builder::Builder, getset::Getters),

derive(derive_builder::Builder, getset::CopyGetters, getset::Getters),

saschagrunert · 2021-08-20T07:36:54Z

src/image/index.rs

+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),
+        builder(default, pattern = "owned", setter(into, strip_option)),
+        getset(get = "pub")


Same for this

Suggested change

getset(get = "pub")

saschagrunert · 2021-08-20T07:37:11Z

src/image/manifest.rs

+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),
+        builder(pattern = "owned", setter(into, strip_option)),
+        getset(get = "pub")


Suggested change

getset(get = "pub")

saschagrunert · 2021-08-20T07:37:24Z

src/image/manifest.rs

+    #[serde(rename_all = "camelCase")]
+    #[cfg_attr(
+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),


Suggested change

derive(derive_builder::Builder, getset::Getters),

derive(derive_builder::Builder, getset::CopyGetters, getset::Getters),

saschagrunert · 2021-08-20T07:37:31Z

src/image/index.rs

+    #[serde(rename_all = "camelCase")]
+    #[cfg_attr(
+        feature = "builder",
+        derive(derive_builder::Builder, getset::Getters),


Suggested change

derive(derive_builder::Builder, getset::Getters),

derive(derive_builder::Builder, getset::CopyGetters, getset::Getters),

saschagrunert · 2021-08-20T07:38:15Z

test_files/image/config.json

@@ -0,0 +1,52 @@
+{


How about putting those files into the official test directory, like: test/data?

Furisto · 2021-08-20T16:52:15Z

@saschagrunert PTAL
I included your suggestions and also formatted the comments to have a uniform width of 80 characters.
Does intellisense work with the generated getters for you? I tried enabling the required feature in the settings of rust-analyzer and even created a second project and added this crate as a dependency but it does not get picked up. Cargo expand confirms that everything has been generated as expected and I can use them and it compiles fine, but it does not show up in intellisense. This happens for both runtime and image.

saschagrunert · 2021-08-20T18:08:35Z

Thanks! Rustfmt allows 100chars but having 80 does not hurt. I’m using vim with YouCompleteMe and the rust analyzer. This works pretty well from my experience, unfortunately I don’t know how it behaves with intellisense.

saschagrunert reviewed Aug 20, 2021

View reviewed changes

Furisto force-pushed the image branch from 8cf7e46 to f04c144 Compare August 20, 2021 16:32

Furisto added 8 commits August 20, 2021 18:34

Initial implementation of image spec

f9ae673

Add builders for image spec

225319d

Improve docs

996b508

Add default for builder

1596ae8

Fix duplicate doc comment

b3d7ddc

Add feature flag

d1d5098

Clippy

065512d

Incorporate review feedback

7b91723

Furisto force-pushed the image branch from f04c144 to 8014f32 Compare August 20, 2021 16:35

Format comments

5e8fb25

Furisto force-pushed the image branch from 8014f32 to 5e8fb25 Compare August 20, 2021 16:36

saschagrunert approved these changes Aug 20, 2021

View reviewed changes

saschagrunert merged commit a3a9eb1 into containers:main Aug 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement image spec #25

Implement image spec #25

Furisto commented Aug 19, 2021

codecov-commenter commented Aug 19, 2021

saschagrunert left a comment

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

saschagrunert Aug 20, 2021

Furisto commented Aug 20, 2021 •

edited

saschagrunert commented Aug 20, 2021

		use serde::ser::SerializeMap;
		use serde::{Deserialize, Deserializer, Serialize, Serializer};

	empty_layer: Option<bool>,
	#[cfg_attr(feature = "builder", getset(get_copy = "pub"))]
	empty_layer: Option<bool>,

	derive(derive_builder::Builder, getset::Getters),
	derive(derive_builder::Builder, getset::CopyGetters, getset::Getters),

Implement image spec #25

Implement image spec #25

Conversation

Furisto commented Aug 19, 2021

codecov-commenter commented Aug 19, 2021

Codecov Report

saschagrunert 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Furisto commented Aug 20, 2021 • edited

saschagrunert commented Aug 20, 2021

Furisto commented Aug 20, 2021 •

edited