Skip to content
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

feat: json-parse promotion step #3327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

fykaa
Copy link
Contributor

@fykaa fykaa commented Jan 21, 2025

closes #3240

parse-file step allows users to specify file paths and extract specific values using expressions. The fromExpression field accepts direct expressions without ${{}} wrappers, as evaluation happens within the step logic itself.

eg.

steps:
  - uses: json-parse
    config:
      path: "path/to/file1.json"
      outputs:
        - fromExpression: "object.version"
          name: "version1"
        - fromExpression: "object.name"
          name: "name1"

@fykaa fykaa self-assigned this Jan 21, 2025
@fykaa fykaa requested a review from a team as a code owner January 21, 2025 12:52
@fykaa fykaa marked this pull request as draft January 21, 2025 12:52
Copy link

netlify bot commented Jan 21, 2025

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit bbd26bc
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67aaf7a568b5c300083d5583
😎 Deploy Preview https://deploy-preview-3327.docs.kargo.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Jan 21, 2025

Codecov Report

Attention: Patch coverage is 80.55556% with 14 lines in your changes missing coverage. Please review.

Project coverage is 52.33%. Comparing base (4e5d4bc) to head (927753e).

Files with missing lines Patch % Lines
internal/directives/json_parser.go 80.55% 13 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3327      +/-   ##
==========================================
+ Coverage   52.25%   52.33%   +0.07%     
==========================================
  Files         290      291       +1     
  Lines       26306    26378      +72     
==========================================
+ Hits        13747    13805      +58     
- Misses      11809    11822      +13     
- Partials      750      751       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@krancour
Copy link
Member

Sorry for the delay in the latest round of reviews here.

The schema is looking pretty good. 👍

I appreciate us reaching consensus on that before starting on the implementation.

There's another detail that's just occurred to me, and I'd like to invite @hiddeco and @jessesuen to weight in.

Had we been thinking of a single parse-file step that can handle both JSON and YAML? Or had we been thinking of two similar, but separate steps? thb, I'd not even considered this question until now.

I think I may have a slight preference for separate parse-yaml and parse-json steps, with my rationale being: On a technical level, it seems easy enough to combine these, but if we add support for parsing other specific types of files in the future, their general shape might not conform to this schema that happens to work well for both JSON and YAML. At that point, we might regret having named a step so generically as parse-file.

I wouldn't die on this hill.

@krancour
Copy link
Member

Had we been thinking of a single parse-file step that can handle both JSON and YAML? Or had we been thinking of two similar, but separate steps?

@jessesuen and I discussed this offline and agreed on separate steps (which can share a lot of common code underneath).

They can even share the schema.

This plan gives us the flexibility to add file parsers for additional formats in the future which may not conform to the general "shape" of these two.

@fykaa
Copy link
Contributor Author

fykaa commented Jan 31, 2025

The schema is looking pretty good. 👍

@krancour I'm still considering a couple of different conditions. Before proceeding i wanted to confirm:

Do we need to support an array of files in the schema or should we assume that the json-parse step processes only one file at a time?

for instance, instead of this approach:

steps:
  - uses: json-parse
    config:
      json-files:
        - path: "path/to/file1.json"
          outputs:
            - fromExpression: "object.version"
              name: "version1"
            - fromExpression: "object.name"
              name: "name1"
        - path: "path/to/file2.json"
          outputs:
            - fromExpression: "object.version"
              name: "version2"
            - fromExpression: "object.name"
              name: "name2"

with the corresponding config structure:

type JSONParseConfig struct {
	JSONFiles []JSONFile `json:"jsonFiles"`
}

type JSONFile struct {
	Outputs []JSONParse `json:"outputs"`
	Path    string      `json:"path"`
}

type JSONParse struct {
	FromExpression string `json:"fromExpression"`
	Name           string `json:"name"`
}

should we instead use a simpler structure like this?

steps:
  - uses: json-parse
    config:
      path: "path/to/file1.json"
      outputs:
        - fromExpression: "object.version"
          name: "version1"
        - fromExpression: "object.name"
          name: "name1"

with the corresponding config:

type JSONParseConfig struct {
	Path    string      `json:"path"`
	Outputs []JSONParse `json:"outputs"`
}

type JSONParse struct {
	FromExpression string `json:"fromExpression"`
	Name           string `json:"name"`
}

the second approach keeps it simple, and I don’t see a strong reason to introduce an object for multiple files. can you confirm if this aligns with the intended design?

also i think keeping it this way maintains consistency with other steps like json-update

@hiddeco
Copy link
Contributor

hiddeco commented Feb 3, 2025

Single-file multi-output is likely the best option to keep things simple (i.e., the second option).

@krancour
Copy link
Member

krancour commented Feb 3, 2025

Agree with @hiddeco. Most existing steps deal with one file (or one thing -- one HTTP request, one PR, etc.) at a time. argocd -update seems like a conspicuous exception to that, but in that case, I think there are good reasons for it.

@fykaa fykaa force-pushed the feat/parse-files-promotion-step branch from a4de8cf to 8154259 Compare February 6, 2025 17:51
@fykaa fykaa marked this pull request as ready for review February 6, 2025 17:53
@fykaa fykaa requested a review from a team as a code owner February 6, 2025 18:05
@fykaa fykaa force-pushed the feat/parse-files-promotion-step branch from 955ad18 to b439000 Compare February 6, 2025 23:23
@fykaa fykaa changed the title feat: parse-file promotion step feat: json-parse promotion step Feb 7, 2025
@fykaa fykaa force-pushed the feat/parse-files-promotion-step branch from 34c6326 to b439000 Compare February 7, 2025 19:47
Signed-off-by: Faeka Ansari <[email protected]>
Signed-off-by: Faeka Ansari <[email protected]>
@fykaa fykaa force-pushed the feat/parse-files-promotion-step branch from d116907 to 7b7368c Compare February 11, 2025 06:52
Signed-off-by: Faeka Ansari <[email protected]>

docs

Signed-off-by: Faeka Ansari <[email protected]>

nit

docs

Signed-off-by: Faeka Ansari <[email protected]>
@fykaa fykaa force-pushed the feat/parse-files-promotion-step branch from 7b7368c to 927753e Compare February 11, 2025 06:57
Signed-off-by: Faeka Ansari <[email protected]>
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.

Parse (expr) step to extract JSON/YAML file contents
3 participants