Skip to content

SYS-187 update build process #37

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

Merged
merged 1 commit into from
Jul 16, 2024
Merged

Conversation

TieDyedSheep
Copy link
Contributor

remove ignore-scripts and explicit build step
add some explanatory output stuff
updated CI script base to sync it (testing some codium changes with this, has no impact on the CI process)

Copy link

linear bot commented Jul 16, 2024

Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Redundant Output
The addition of environment variable checks (e.g., IS_LINT_REQUIRED) directly after error messages might be redundant if these variables are not used to conditionally control the flow or output significantly. Consider simplifying the output or using these variables in a meaningful way to control the workflow.

Workflow Configuration
The new inputs configuration for workflowBranch in the issue_comment trigger might need additional validation or error handling to ensure that the workflow behaves as expected when triggered by comments.

@chrypnotoad
Copy link
Contributor

/improve

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Score
Possible issue
Ensure the IS_LINT_REQUIRED environment variable is defined before use

Add a check to ensure that the IS_LINT_REQUIRED environment variable is defined
before using it in a conditional statement to avoid potential runtime errors.

.github/workflows/reusable-node-ci.yml [144-145]

-if ${{ env.IS_LINT_REQUIRED }}; then
+if [ -z "${{ env.IS_LINT_REQUIRED }}" ]; then
+  echo "IS_LINT_REQUIRED is not set."
+elif ${{ env.IS_LINT_REQUIRED }}; then
   failed=true
 
Suggestion importance[1-10]: 8

Why: Adding a check for the IS_LINT_REQUIRED variable can prevent potential runtime errors, making this a significant improvement for robustness.

8
Maintainability
Refactor repeated echo statements into a single function for better maintainability

Consolidate the echo statements related to required checks into a single function or
script to improve code reuse and maintainability.

.github/workflows/reusable-node-ci.yml [143-170]

-echo "IS_LINT_REQUIRED = ${{ env.IS_LINT_REQUIRED }}"
-echo "IS_FORMAT_CHECK_REQUIRED = ${{ env.IS_FORMAT_CHECK_REQUIRED }}"
-echo "IS_APPLY_PATCHES_REQUIRED = ${{ env.IS_APPLY_PATCHES_REQUIRED }}"
-echo "IS_UNIT_TESTS_REQUIRED = ${{ env.IS_UNIT_TESTS_REQUIRED }}"
+echo_required_vars() {
+  echo "IS_LINT_REQUIRED = ${{ env.IS_LINT_REQUIRED }}"
+  echo "IS_FORMAT_CHECK_REQUIRED = ${{ env.IS_FORMAT_CHECK_REQUIRED }}"
+  echo "IS_APPLY_PATCHES_REQUIRED = ${{ env.IS_APPLY_PATCHES_REQUIRED }}"
+  echo "IS_UNIT_TESTS_REQUIRED = ${{ env.IS_UNIT_TESTS_REQUIRED }}"
+}
+echo_required_vars
 
Suggestion importance[1-10]: 7

Why: Consolidating echo statements into a function improves code reuse and maintainability, but it is not a critical change.

7
Streamline echo statements in the installation step

Replace the verbose echo statements with a single line that encapsulates both
actions to streamline the output and reduce clutter in the logs.

.github/workflows/reusable-node-ci.yml [89-91]

-echo "Install and build..."
+echo "Installing dependencies and building..."
 npm ci
-echo "Install and Build complete."
+echo "Dependencies installed and build complete."
 
Suggestion importance[1-10]: 5

Why: The suggestion to streamline echo statements is valid for reducing log clutter, but it is a minor improvement in terms of maintainability.

5
Enhancement
Make the workflowBranch input under issue_comment optional

Consider setting the required field to false for the workflowBranch input under
issue_comment. This allows for more flexibility, especially if the workflow can
intelligently pick a default branch without user input.

.github/workflows/ci.yml [18]

-required: true
+required: false
 
Suggestion importance[1-10]: 6

Why: Making the workflowBranch input optional can provide more flexibility, but it depends on whether the workflow can handle a missing branch input. This is a minor enhancement.

6

@TieDyedSheep TieDyedSheep merged commit 030605d into dev Jul 16, 2024
7 checks passed
@TieDyedSheep TieDyedSheep deleted the SYS-187-update-install-and-build branch July 16, 2024 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants