Skip to content

freeze ndc-lambda-sdk version#100

Merged
m-Bilal merged 6 commits intomainfrom
m-bilal/freeze-ndc-lambda-sdk-version
Jul 21, 2025
Merged

freeze ndc-lambda-sdk version#100
m-Bilal merged 6 commits intomainfrom
m-bilal/freeze-ndc-lambda-sdk-version

Conversation

@m-Bilal
Copy link
Copy Markdown
Member

@m-Bilal m-Bilal commented Jul 21, 2025

No description provided.

@m-Bilal m-Bilal requested a review from Copilot July 21, 2025 19:57

This comment was marked as outdated.

@m-Bilal m-Bilal requested a review from Copilot July 21, 2025 20:25
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR freezes the ndc-nodejs-lambda SDK to version v1.15.0 by replacing dynamic version resolution with a hardcoded version constant. The change ensures version consistency between the SDK version used in generated code and the corresponding Docker image.

  • Replaces dynamic npm registry lookups with a hardcoded version constant
  • Adds a verification script to ensure version consistency across files
  • Removes the pacote dependency that was used for dynamic version resolution

Reviewed Changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/app/context.ts Adds hardcoded SDK version constant and getter method
src/app/writer/package-json-writer.ts Removes dynamic version resolution and uses context version
tests/scripts/verify-ndc-lambda-sdk-version.sh New verification script for version consistency
.github/workflows/test.yaml Adds CI job to run version verification
package.json Removes pacote dependency
changelog.md Documents the version freeze change

Comment on lines +18 to +19
# Extract version from context.ts
local context_version=$(grep -E "const NDC_NODEJS_LAMBDA_SDK_VERSION = " "$CONTEXT_FILE" | sed -E 's/.*"([^"]+)".*/\1/')
Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The regex pattern for extracting the version is fragile and may break if the code formatting changes. Consider using a more robust parsing approach or making the regex more flexible to handle variations in whitespace and formatting.

Suggested change
# Extract version from context.ts
local context_version=$(grep -E "const NDC_NODEJS_LAMBDA_SDK_VERSION = " "$CONTEXT_FILE" | sed -E 's/.*"([^"]+)".*/\1/')
# Extract version from context.ts using a robust JavaScript parser
local context_version=$(node -e "
const fs = require('fs');
const fileContent = fs.readFileSync('$CONTEXT_FILE', 'utf8');
const match = fileContent.match(/const NDC_NODEJS_LAMBDA_SDK_VERSION = ['\"]([^'\"]+)['\"]/);
if (match) {
console.log(match[1]);
} else {
console.error('Error: NDC_NODEJS_LAMBDA_SDK_VERSION not found in $CONTEXT_FILE');
process.exit(1);
}")

Copilot uses AI. Check for mistakes.
local context_version=$(grep -E "const NDC_NODEJS_LAMBDA_SDK_VERSION = " "$CONTEXT_FILE" | sed -E 's/.*"([^"]+)".*/\1/')

# Extract version from Dockerfile
local dockerfile_version=$(grep -E "FROM ghcr.io/hasura/ndc-nodejs-lambda:" "$DOCKERFILE" | sed -E 's/.*:([^[:space:]]+).*/\1/')
Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

The regex pattern assumes a specific FROM statement format and may fail if there are multiple FROM statements or different formatting. Consider making the pattern more specific to avoid false matches.

Suggested change
local dockerfile_version=$(grep -E "FROM ghcr.io/hasura/ndc-nodejs-lambda:" "$DOCKERFILE" | sed -E 's/.*:([^[:space:]]+).*/\1/')
local dockerfile_version=$(grep -E "^\s*FROM\s+ghcr\.io/hasura/ndc-nodejs-lambda:[^[:space:]]+" "$DOCKERFILE" | sed -E 's/^\s*FROM\s+ghcr\.io\/hasura\/ndc-nodejs-lambda:([^[:space:]]+).*/\1/')

Copilot uses AI. Check for mistakes.
echo "=== Checking if npm package version is downloadable ==="

# Extract version from context.ts (remove 'v' prefix if present)
local context_version=$(grep -E "const NDC_NODEJS_LAMBDA_SDK_VERSION = " "$CONTEXT_FILE" | sed -E 's/.*"([^"]+)".*/\1/' | sed 's/^v//')
Copy link

Copilot AI Jul 21, 2025

Choose a reason for hiding this comment

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

This line duplicates the version extraction logic from line 19. Consider extracting this into a reusable function to avoid code duplication and ensure consistency.

Suggested change
local context_version=$(grep -E "const NDC_NODEJS_LAMBDA_SDK_VERSION = " "$CONTEXT_FILE" | sed -E 's/.*"([^"]+)".*/\1/' | sed 's/^v//')
local context_version=$(extract_context_version | sed 's/^v//')

Copilot uses AI. Check for mistakes.
@m-Bilal m-Bilal merged commit 21d7d21 into main Jul 21, 2025
5 checks passed
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