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

feat: wasmer-config #4582

Merged
merged 4 commits into from
Apr 17, 2024
Merged

feat: wasmer-config #4582

merged 4 commits into from
Apr 17, 2024

Conversation

theduke
Copy link
Contributor

@theduke theduke commented Apr 17, 2024

Add a new wasmer-config create, which:

  • Supersedes the wasmer-toml crate
  • Includes the app.yaml configuration schema

Compared to the original upstream wasmer-toml, this PR includes the
following additions:

  • new types for packages (PackageSource, PackageId, PackageIdent,
    Sha256Hash, PackageHash, ...)
  • AppConfigV1, which describes the format of app.yaml files

The motivation for inclusion is to:

  • Simplify management so we don't have to maintain another repository
  • Expand the scope of the crate to include additional Wasmer related
    configuration - we would oterhwise need to maintain additional crates

The PR also adds a new Github workflow for checking the crate.

Closes #4553

@theduke theduke requested review from syrusakbary and xdoardo April 17, 2024 11:52
Add a new wasmer-config create, which:

* Supersedes the wasmer-toml crate
* Includes the app.yaml configuration schema

Compared to the original upstream wasmer-toml, this PR includes the
following additions:

* new types for packages (PackageSource, PackageId, PackageIdent,
  Sha256Hash, PackageHash, ...)
* `AppConfigV1`, which describes the format of `app.yaml` files

The motivation for inclusion is to:

* Simplify management so we don't have to maintain another repository
* Expand the scope of the crate to include additional Wasmer related
  configuration - we would oterhwise need to maintain additional crates

The PR also adds a new Github workflow for checking the crate.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that this file could be named named_package.rs or - not ideal, I know - to keep the congruence with the names of other files, package_named.rs. The rationale for this is that it'd be easy to confuse this file (thus module as well) with package_ident.rs.

Copy link
Contributor Author

@theduke theduke Apr 17, 2024

Choose a reason for hiding this comment

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

The main type in the module is PackageId (and NamedPackageId), so that name suggestion seems more confusing to me.

The duplication between PackageId and Ident is suboptimal indeed, but one is a loose specification with version ranges etc, while the other is just a concrete single package version.

Copy link
Member

@syrusakbary syrusakbary left a comment

Choose a reason for hiding this comment

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

It looks good in general, some comments:

  • Any dependencies that are used in other crates should be a workspace dependency
  • Delete commented code
  • Other minor comments

lib/config/src/package/package_id.rs Outdated Show resolved Hide resolved
lib/config/Cargo.toml Outdated Show resolved Hide resolved
lib/config/Cargo.toml Show resolved Hide resolved
lib/config/src/package/mod.rs Show resolved Hide resolved
@theduke theduke merged commit f5b182f into master Apr 17, 2024
55 of 56 checks passed
@theduke theduke deleted the wasmer-config branch April 17, 2024 13:53
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.

Introduce new wasmer-configs crate
3 participants