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 configuration management package #16

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

GuiBL4
Copy link
Contributor

@GuiBL4 GuiBL4 commented May 19, 2023

Description

This pull request aims at refactoring the configuration crate to be more generic. The configuration crate is, nowadays, reading a configuration file to retrieve the given release rules. However, we want this configuration to be more generic and load all the data and/or file needed by the crates.
We decided that each crate that needs file and/or data must implement its own configuration.

Three structures need to be created :

  • One named Context with a field map which is a HashMap<String, Value>.
  • One named Configuration with a field map wich is a HashMap<String, Value>, where the key defines the data to access.
  • One name Value which represents all possible value a data could be. Therefore, a method to access the value is also required like as_string(), as_commits(), etc.

The crate that needs to access Context are :

  • sleppa_commit_analyzer: it needs acces to the list of commits and an optional toml file to read the release rules and apply them.
  • sleppa_changelog: it needs a logged user to commit the CHANGELOG.md into the repository, the repository, the last tag and new tag and the list of commits. An optional changlof file path can be provided by the user and then loaded.
  • sleppa_code_archiver: it needs a logged user to publish the release into the repository
  • sleppa_versioner: it needs the last tag and the release action type.

The Context implements method to retrieve these Values.

Related issues

closes #10

How has this been tested?

The crates contain unit tests in the file tests.rs.

Test configuration:

  • Firmware version: Ubuntu 22.10
  • Hardware: Intel i5-5200U, 4.0GiB RAM
  • Toolchain: Rust cargo test
  • SDK: -

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@GuiBL4 GuiBL4 marked this pull request as ready for review May 30, 2023 14:58
Copy link
Contributor

@binadamu-isiyoonekana binadamu-isiyoonekana left a comment

Choose a reason for hiding this comment

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

Let's also discuss at 10:00 as there's a lot in the PR :) Bravo Guillaume

crates/sleppa_changelog/src/constants.rs Outdated Show resolved Hide resolved
crates/sleppa_code_archiver/src/lib.rs Show resolved Hide resolved
crates/sleppa_commit_analyzer/sleppa_commit_analyzer.toml Outdated Show resolved Hide resolved
crates/sleppa_commit_analyzer/sleppa_commit_analyzer.toml Outdated Show resolved Hide resolved
crates/sleppa_commit_analyzer/src/lib.rs Outdated Show resolved Hide resolved
crates/sleppa_configuration/src/lib.rs Outdated Show resolved Hide resolved
crates/sleppa_versioner/src/lib.rs Show resolved Hide resolved
crates/sleppa_versioner/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@binadamu-isiyoonekana binadamu-isiyoonekana left a comment

Choose a reason for hiding this comment

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

Hello Guillaume. As stated earlier in MM, the context implementation is too static and not extensible. It must be refactored. Can you please take a look at the tiny crate I quickly implemented here. Thanks a lot.

where
R: GitRepository,
{
pub map: HashMap<String, Value>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on public members

/// The context structure used to share datas between crates.
///
/// The used repository should implements the [GitRepository] trait as Sleppa works only with git.
pub struct Context<R>
Copy link
Contributor

Choose a reason for hiding this comment

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

More readable if the context module is implemented in its own context.rs file.

crates/sleppa_primitives/src/lib.rs Show resolved Hide resolved
crates/sleppa_primitives/src/lib.rs Show resolved Hide resolved
crates/sleppa_primitives/src/lib.rs Show resolved Hide resolved

## How it works
These `commits` are given to the `sleppa_commit_analyzer` which will analyze their message. In order to proceed, a configuration file that defines the release rules is loaded.
The crate determines the new release action type to apply.
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit analyzer aims at determining the new release action type to apply, namely a [Minor] , [Major] or [Patch] action.

crates/sleppa_primitives/src/lib.rs Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: status: in progress
Development

Successfully merging this pull request may close these issues.

Refactor configuration management package
2 participants