Skip to content

chore: add muse dev config#39

Open
beeme1mr wants to merge 2 commits intomainfrom
add-muse-config
Open

chore: add muse dev config#39
beeme1mr wants to merge 2 commits intomainfrom
add-muse-config

Conversation

@beeme1mr
Copy link
Copy Markdown
Member

Add a Muse config so we can test the process proposed here.

  • Adds a muse config
  • Updates the CI to use the new config
  • Bump the container image version
  • Adds contrib docs

Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a CONTRIBUTING.md guide, adds a mise.toml file for tool version management, and updates the project to Node.js 20 across the Dockerfile and mise configuration. It also adds a renovate.json configuration. Feedback includes a suggestion to update the OpenFeature CLI version to 0.7.1, a reminder to synchronize the .nvmrc file with the new Node version, and a recommendation to include mise activation instructions in the setup guide to ensure the correct environment is used.

Comment thread CONTRIBUTING.md
1. [Install mise](https://mise.jdx.dev/getting-started.html)
2. Install project tools:
```sh
mise install
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The mise setup instructions are missing the activation step. Without activating mise in the shell (e.g., via eval "$(mise activate bash)" or the shell hook), subsequent commands like npm install and npm run dev will use the system's default Node.js version instead of the one pinned in mise.toml. Consider adding a note about activation or recommending the use of mise exec for these commands.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Gemini is referring to https://mise.jdx.dev/getting-started.html#activate-mise, which we should add to the instructions as well.

Comment thread CONTRIBUTING.md
npm run dev
```

If you prefer nvm, the `.nvmrc` file is still present. You'll need to [install the OpenFeature CLI](https://github.com/open-feature/cli#installation) separately.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The documentation states that .nvmrc is still present. Since the project is upgrading to Node 20 (as seen in the Dockerfile and mise.toml changes), please ensure that .nvmrc is also updated to 20 to maintain consistency for contributors using nvm. It appears that .nvmrc was not updated in this pull request, which could lead to version mismatches.

Comment thread mise.toml
@@ -0,0 +1,4 @@
[tools]
node = "20"
"github:open-feature/cli" = "0.4.1"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The OpenFeature CLI version 0.4.1 is quite old. Given that the project uses recent versions of the OpenFeature SDKs (e.g., @openfeature/server-sdk ^1.18.0), it is recommended to update to a more recent version (e.g., 0.7.1) to ensure compatibility and access to the latest features and bug fixes.

Suggested change
"github:open-feature/cli" = "0.4.1"
"github:open-feature/cli" = "0.7.1"

Signed-off-by: Michael Beemer <beeme1mr@users.noreply.github.com>
@sahidvelji
Copy link
Copy Markdown

Not necessary, but if you wanted to go all-in on mise, you could get rid of the Makefile and use mise tasks. Similarly for environment variables (instead of .env* files), you could use https://mise.jdx.dev/environments/.

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.

2 participants