Skip to content

Conversation

@rswanson
Copy link
Member

No description provided.

@rswanson rswanson mentioned this pull request May 20, 2025
Copy link
Contributor

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 introduces a shared utils package to centralize environment-variable mapping and Kubernetes ConfigMap creation, reducing duplication across the signet_node and builder modules.

  • Add pkg/utils/env.go with CreateEnvMap, CreateConfigMap, CamelToSnake, and conversion helpers, plus corresponding tests.
  • Refactor signet_node and builder packages to use utils.CreateEnvMap/CreateConfigMap and implement EnvProvider, removing legacy helpers.
  • Update types, tests, and APIs (e.g., BuilderEnv.BuilderPort now a StringInput) to align with the new utils.

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
pkg/utils/env.go New DRY utilities for converting structs to env maps and ConfigMaps
pkg/utils/env_test.go Tests covering CamelToSnake, getStringValue, CreateEnvMap, and interface usage
pkg/signet_node/types.go Implement EnvProvider for SignetNodeEnv using the new utils
pkg/signet_node/signet_node.go Replace manual ConfigMap creation with utils.CreateConfigMap
pkg/signet_node/helpers_test.go Removed old env mapping tests (covered now in utils)
pkg/builder/types.go Change BuilderEnv.BuilderPort to StringInput and add GetEnvMap
pkg/builder/validation_test.go Update validation tests to use string port inputs
pkg/builder/builder.go Use utils.CreateConfigMap and EnvFrom instead of manual env logic
Comments suppressed due to low confidence (2)

pkg/signet_node/signet_node.go:91

  • [nitpick] The name exex-configmap is non-intuitive; consider renaming it to something like execution-configmap for clarity.
"exex-configmap",

pkg/signet_node/helpers_test.go:7

  • Tests for the new EnvProvider behavior in SignetNodeEnv were removed; consider adding integration tests to ensure utils.CreateEnvMap produces the expected environment variables for SignetNodeEnv.
import (

@rswanson rswanson mentioned this pull request May 27, 2025
@rswanson rswanson requested review from Evalir and dylanlott May 27, 2025 17:00
This was referenced May 27, 2025
@rswanson rswanson force-pushed the swanny/utils-are-dry branch from b7d5735 to 4bfa0b0 Compare May 29, 2025 11:25
@rswanson rswanson force-pushed the swanny/signet-node-component branch from 4741fb5 to 28c4e58 Compare May 29, 2025 11:25
@rswanson rswanson force-pushed the swanny/utils-are-dry branch from 4bfa0b0 to 8cf02b7 Compare May 29, 2025 11:29
@rswanson rswanson self-assigned this May 29, 2025
Copy link
Member Author

rswanson commented May 29, 2025

Merge activity

  • May 29, 1:49 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • May 29, 1:50 PM UTC: Graphite rebased this pull request as part of a merge.
  • May 29, 1:52 PM UTC: @rswanson merged this pull request with Graphite.

@rswanson rswanson changed the base branch from swanny/signet-node-component to graphite-base/3 May 29, 2025 13:49
@rswanson rswanson changed the base branch from graphite-base/3 to main May 29, 2025 13:49
@rswanson rswanson force-pushed the swanny/utils-are-dry branch from 8cf02b7 to 18bb794 Compare May 29, 2025 13:50
@rswanson rswanson merged commit c52fec4 into main May 29, 2025
2 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.

3 participants