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

Refactor models to reduce duplication #591

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Refactor models to reduce duplication #591

merged 1 commit into from
Dec 17, 2019

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Dec 17, 2019

To be able to have models/lib.rs with shared code, we needed to stop symlinking the entire src directory and instead treat the variant-specific code as Rust modules, symlinking the correct one to variant::current so that lib.rs can re-export Settings. (We also have to symlink in variant/mod.rs so Rust knows it's part of the module hierarchy.)

Now, each variant defines a Settings structure using common sub-structures. Those sub-structures live in the main lib.rs, and the modeled types are now in a shared modeled_types module. No changes were made to the code, just its organization.

Documentation was updated to reflect these changes, including using cargo-readme to generate README.md now that there's a single source.


Reviewer's note: regarding the giant green/red code blocks - nothing in the implementation changed, it's just in a different place.

Testing done:

Confirmed that all workspaces/ unit tests pass, whether for aws-dev or aws-k8s.

Confirmed that local builds still create the symlink(s) correctly and get the right model.

Made an aws-k8s AMI, confirmed that the containerd config still has the CRI settings. It connected to my cluster and ran a pod OK.

Made an aws-dev AMI, confirmed that the containerd config was the smaller dev version. Ran a docker container OK.

To be able to have `models/lib.rs` with shared code, we needed to stop symlinking the entire `src` directory and instead treat the variant-specific code as Rust modules, symlinking the correct one to `variant::current` so that `lib.rs` can re-export `Settings`.  (We also have to symlink in `variant/mod.rs` so Rust knows it's part of the module hierarchy.)

Now, each variant defines a `Settings` structure using common sub-structures.  Those sub-structures live in the main `lib.rs`, and the modeled types are now in a shared `modeled_types` module.  No changes were made to the code, just its organization.

Documentation was updated to reflect these changes, including using cargo-readme to generate README.md now that there's a single source.
@tjkirch
Copy link
Contributor Author

tjkirch commented Dec 17, 2019

To help visualize the new structure:

├── build.rs
├── Cargo.toml
├── README.md
├── README.tpl
└── src
    ├── aws-dev
    │   ├── defaults.toml
    │   └── mod.rs
    ├── aws-k8s
    │   ├── defaults.toml
    │   └── mod.rs
    ├── lib.rs
    ├── modeled_types
    │   ├── kubernetes.rs
    │   ├── mod.rs
    │   └── shared.rs
    ├── variant
    │   ├── current -> ../aws-dev
    │   └── mod.rs -> ../variant_mod.rs
    └── variant_mod.rs

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

This is super cool

🎁

@tjkirch tjkirch merged commit 6edfc28 into develop Dec 17, 2019
@tjkirch tjkirch deleted the models-refactor branch December 17, 2019 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants