Rename Secret.env_name to Secret.env_var #6176

thomasjpfan · 2025-01-16T17:56:27Z

Tracking issue

Follow up to #6160

Why are the changes needed?

env_var is a better name than env_name.

What changes were proposed in this pull request?

This PR renames env_name to env_var.

How was this patch tested?

I used flyteorg/flytekit#3048 for testing.

Summary by Bito

This PR implements a consistent renaming of the Secret message field from 'env_name' to 'env_var' across all generated protobuf code files. The change affects multiple language implementations including TypeScript, Go, JavaScript, Python, and Rust, updating field names, method signatures, and type definitions. The modification aims to improve clarity by better indicating that the field represents an environment variable.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 3

flyte-bot · 2025-01-16T17:56:52Z

Code Review Agent Run #ea2a6d

Actionable Suggestions - 1

flyteidl/gen/pb_rust/flyteidl.core.rs - 1
- Consider backward compatibility for field rename · Line 1057-1057

Review Details

Files reviewed - 11 · Commit Range: 25a7d9c..25a7d9c
- flyteidl/gen/pb-es/flyteidl/core/security_pb.ts
- flyteidl/gen/pb-go/flyteidl/core/security.pb.go
- flyteidl/gen/pb-js/flyteidl.d.ts
- flyteidl/gen/pb-js/flyteidl.js
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.py
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi
- flyteidl/gen/pb_rust/flyteidl.core.rs
- flyteidl/protos/flyteidl/core/security.proto
- flytepropeller/pkg/webhook/k8s_secrets.go
- flytepropeller/pkg/webhook/k8s_secrets_test.go
- flytepropeller/pkg/webhook/utils.go
Files skipped - 4
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/agent.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/external_plugin_service.swagger.json - Reason: Filter setting
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-16T18:06:20Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - Rename Secret Field from env_name to env_var	- `security_pb.ts` - Updated field name from envName to envVar and related comments - `security.pb.go` - Changed field name from EnvName to EnvVar including method signatures and comments - `flyteidl.d.ts` - Modified type definitions to use envVar instead of envName - `flyteidl.js` - Updated JavaScript implementation to use envVar field name - `security_pb2.py` - Updated Python protobuf descriptor and serialization details - `security_pb2.pyi` - Modified Python type hints to use env_var instead of env_name - `flyteidl.core.rs` - Updated Rust implementation to use env_var field name
Feature Improvement - Rename Secret Field from env_name to env_var	- `security.proto` - Updated field name and comments from env_name to env_var in proto definition - `k8s_secrets.go` - Updated method calls and field references from GetEnvName to GetEnvVar - `k8s_secrets_test.go` - Updated test cases to use EnvVar instead of EnvName - `utils.go` - Changed secret field access from GetEnvName to GetEnvVar - `security_pb.ts` - Updated field name from envName to envVar and related comments - `security.pb.go` - Changed field name from EnvName to EnvVar including method signatures and comments - `flyteidl.d.ts` - Modified type definitions to use envVar instead of envName - `flyteidl.js` - Updated JavaScript implementation to use envVar field name - `security_pb2.py` - Updated Python protobuf descriptor and serialization details - `security_pb2.pyi` - Modified Python type hints to use env_var instead of env_name - `flyteidl.core.rs` - Updated Rust implementation to use env_var field name

flyte-bot · 2025-01-16T18:06:22Z

flyteidl/gen/pb_rust/flyteidl.core.rs

    /// then the value is the secret itself. If mount_requirement is FILE, then the value is the path to the secret file.
    /// +optional
    #[prost(string, tag="5")]
-    pub env_name: ::prost::alloc::string::String,
+    pub env_var: ::prost::alloc::string::String,


Consider backward compatibility for field rename

Consider if renaming env_name to env_var maintains backward compatibility. This change could potentially break existing code that relies on the field name env_name. Similar issues were also found in:

flyteidl/gen/pb-go/flyteidl/core/security.pb.go (line 152)

flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi (line 23)

Code suggestion

Check the AI-generated fix before applying

Suggested change

pub env_var: ::prost::alloc::string::String,

#[deprecated]

pub env_name: ::prost::alloc::string::String,

pub env_var: ::prost::alloc::string::String,

Code Review Run #ea2a6d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

Signed-off-by: Thomas J. Fan <[email protected]>

flyte-bot · 2025-01-16T19:12:28Z

Code Review Agent Run #1bcd2d

Actionable Suggestions - 2

flyteidl/gen/pb-es/flyteidl/core/security_pb.ts - 1
- Consider impact of field name change · Line 65-65
flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi - 1
- Field number constant name change · Line 23-23

Review Details

Files reviewed - 11 · Commit Range: 30a91e4..30a91e4
- flyteidl/gen/pb-es/flyteidl/core/security_pb.ts
- flyteidl/gen/pb-go/flyteidl/core/security.pb.go
- flyteidl/gen/pb-js/flyteidl.d.ts
- flyteidl/gen/pb-js/flyteidl.js
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.py
- flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi
- flyteidl/gen/pb_rust/flyteidl.core.rs
- flyteidl/protos/flyteidl/core/security.proto
- flytepropeller/pkg/webhook/k8s_secrets.go
- flytepropeller/pkg/webhook/k8s_secrets_test.go
- flytepropeller/pkg/webhook/utils.go
Files skipped - 4
- flyteidl/clients/go/assets/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/admin.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/agent.swagger.json - Reason: Filter setting
- flyteidl/gen/pb-go/gateway/flyteidl/service/external_plugin_service.swagger.json - Reason: Filter setting
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-01-16T19:22:37Z

flyteidl/gen/pb-es/flyteidl/core/security_pb.ts

   */
-  envName = "";
+  envVar = "";


Consider impact of field name change

The field name envVar seems to be renamed from envName, but this change might impact existing code that depends on this field name. Consider if this is an intentional breaking change. Similar issues were also found in:

flyteidl/gen/pb_rust/flyteidl.core.rs (line 1057)

flyteidl/gen/pb-go/flyteidl/core/security.pb.go (line 152)

flyteidl/protos/flyteidl/core/security.proto (line 47)

flyteidl/protos/flyteidl/core/security.proto (line 50)

Code suggestion

Check the AI-generated fix before applying

Suggested change

envVar = "";

envName = "";

Code Review Run #1bcd2d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

flyte-bot · 2025-01-16T19:22:40Z

flyteidl/gen/pb_python/flyteidl/core/security_pb2.pyi

@@ -20,13 +20,13 @@ class Secret(_message.Message):
    GROUP_VERSION_FIELD_NUMBER: _ClassVar[int]
    KEY_FIELD_NUMBER: _ClassVar[int]
    MOUNT_REQUIREMENT_FIELD_NUMBER: _ClassVar[int]
-    ENV_NAME_FIELD_NUMBER: _ClassVar[int]
+    ENV_VAR_FIELD_NUMBER: _ClassVar[int]


Field number constant name change

Consider if renaming ENV_NAME_FIELD_NUMBER to ENV_VAR_FIELD_NUMBER maintains backward compatibility. This change could potentially break existing code that references the old field name.

Code suggestion

Check the AI-generated fix before applying

Suggested change

ENV_VAR_FIELD_NUMBER: _ClassVar[int]

ENV_NAME_FIELD_NUMBER: _ClassVar[int] # Deprecated: Use ENV_VAR_FIELD_NUMBER

ENV_VAR_FIELD_NUMBER: _ClassVar[int]

Code Review Run #1bcd2d

Is this a valid issue, or was it incorrectly flagged by the Agent?

it was incorrectly flagged

thomasjpfan added the added Merged changes that add new functionality label Jan 16, 2025

flyte-bot reviewed Jan 16, 2025

View reviewed changes

Rename Secret.env_namee to Secret.env_var

30a91e4

Signed-off-by: Thomas J. Fan <[email protected]>

thomasjpfan force-pushed the thomasjpfan/rename_to_env_var branch from 25a7d9c to 30a91e4 Compare January 16, 2025 18:07

eapolinario approved these changes Jan 16, 2025

View reviewed changes

flyte-bot reviewed Jan 16, 2025

View reviewed changes

thomasjpfan merged commit 729e71a into flyteorg:master Jan 16, 2025
51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename Secret.env_name to Secret.env_var #6176

Rename Secret.env_name to Secret.env_var #6176

thomasjpfan commented Jan 16, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 16, 2025 •

edited

Loading

Code Review Agent Run #ea2a6d

flyte-bot commented Jan 16, 2025 •

edited

Loading

Changelist by Bito

flyte-bot Jan 16, 2025

flyte-bot commented Jan 16, 2025 •

edited

Loading

Code Review Agent Run #1bcd2d

flyte-bot Jan 16, 2025

flyte-bot Jan 16, 2025

	ENV_VAR_FIELD_NUMBER: _ClassVar[int]
	ENV_NAME_FIELD_NUMBER: _ClassVar[int] # Deprecated: Use ENV_VAR_FIELD_NUMBER
	ENV_VAR_FIELD_NUMBER: _ClassVar[int]

Rename Secret.env_name to Secret.env_var #6176

Rename Secret.env_name to Secret.env_var #6176

Conversation

thomasjpfan commented Jan 16, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Summary by Bito

flyte-bot commented Jan 16, 2025 • edited Loading

Code Review Agent Run #ea2a6d

flyte-bot commented Jan 16, 2025 • edited Loading

Changelist by Bito

flyte-bot Jan 16, 2025

Choose a reason for hiding this comment

flyte-bot commented Jan 16, 2025 • edited Loading

Code Review Agent Run #1bcd2d

flyte-bot Jan 16, 2025

Choose a reason for hiding this comment

flyte-bot Jan 16, 2025

Choose a reason for hiding this comment

thomasjpfan commented Jan 16, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Jan 16, 2025 •

edited

Loading

flyte-bot commented Jan 16, 2025 •

edited

Loading

flyte-bot commented Jan 16, 2025 •

edited

Loading