Skip to content

[FIX] Notebook AWS_REGION in .env and NextGen OpenSearch VPC endpoint - #516

Open
oussamahansal wants to merge 3 commits into
mainfrom
fix/notebook-region-and-nextgen-vpce
Open

[FIX] Notebook AWS_REGION in .env and NextGen OpenSearch VPC endpoint#516
oussamahansal wants to merge 3 commits into
mainfrom
fix/notebook-region-and-nextgen-vpce

Conversation

@oussamahansal

Copy link
Copy Markdown
Collaborator

Description

Two fixes for the lexical-graph CloudFormation templates

Changes

  • Write AWS_REGION into the notebook .env
  • Use a standard EC2 interface VPC endpoint for the NextGen collection

Problem

Related issue (if any): #

Testing

  • Unit tests added/updated
  • Integration tests added (as appropriate)
  • Existing tests pass (pytest)
  • Tested manually (describe below)

Checklist

  • Code follows existing style and conventions
  • License headers present on new files
  • Documentation updated (if applicable)
  • No breaking changes (or clearly documented)

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@mykola-pereyma mykola-pereyma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

On the AWS_REGION change: every template already exports AWS_REGION into the notebook .env on main, so these additions are duplicates (one per template; existing-vpc ends up with three). Details inline. Harmless at runtime but redundant — suggest dropping the AWS_REGION additions unless there's a path where it's actually missing.

(Separately, the NextGen VPC-endpoint type swap is the substantive change and would benefit from confirmation it was deployed end-to-end — the AOSS network policy still references this endpoint in SourceVPCEs, and swapping the AOSS-managed endpoint for a generic EC2 interface endpoint needs to be verified against a live collection.)

@mykola-pereyma mykola-pereyma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For the NextGen VPC-endpoint change (Change 2): it's a structurally sound CloudFormation swap, but its actual functioning depends on two AOSS control-plane behaviors that aren't verifiable from the template. Could you provide deploy confirmation? Specifics inline on the endpoint's ServiceName.

@noel-improv noel-improv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The change looks good, just one blocking item and suggestions referenced below

@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown

Lexical Graph Coverage Report: The coverage is at 65.82% (target: 80%). Download the HTML report here.

@mykola-pereyma mykola-pereyma left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@noel-improv noel-improv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm, thanks for addressing the raised items

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.

3 participants