generate one tree per PR#341
Conversation
| run-go-commands: | ||
| runs-on: ubuntu-latest | ||
| needs: check-changes | ||
| if: ${{ always() && needs.check-changes.outputs.has_tsv_changes == 'true' }} |
There was a problem hiding this comment.
Why do we need always() here?
There was a problem hiding this comment.
Good catch, will remove.
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
Why do we need fetch-depth: 0
There was a problem hiding this comment.
tj-actions/changed-files action needs the entire Git history to accurately calculate which files changed.
| working-directory: ./web/src/main/go | ||
| run: go build -o release-tree ./cmd/release-tree | ||
|
|
||
| - name: Get all changed TSV files |
There was a problem hiding this comment.
Why are we doing this step again?
There was a problem hiding this comment.
I could probably combine into one big git action or pass the files along, but I think this makes the flow clearer at the cost of a pretty quick git action.
| env: | ||
| CHANGED_FILES: ${{ steps.changed-tsv-files.outputs.all_changed_files }} | ||
| run: | | ||
| ./release-tree $CHANGED_FILES |
There was a problem hiding this comment.
Can we use steps.changed-tsv-files.outputs.all_changed_files directly in the run command instead of env variable?
There was a problem hiding this comment.
Since changed files is a whitespace separated list of files, handling it using an env variable causes the action to treat it as a single argument rather than separate arguments.
|
|
||
| - name: Validate mappings | ||
| working-directory: ./web/src/main/go | ||
| run: ./validate-mappings |
There was a problem hiding this comment.
Why not use go run instead?
There was a problem hiding this comment.
This stackoverflow article explains pretty well: https://stackoverflow.com/questions/61060768/why-is-it-recommended-to-use-go-build-instead-of-go-run-when-running-a-go-ap.
Point 3 is the main use for CI/CD
| fmt.Fprintf(os.Stderr, "%v\n", err) | ||
| os.Exit(1) | ||
| } | ||
| mostRecentTree := sortedTreeFiles[len(sortedTreeFiles)-2] // most recent will be the one just uploaded |
There was a problem hiding this comment.
This comment seems misleading. mostRecentTree is the second-to-last tree but seems like it's the new tree that was just uploaded.
There was a problem hiding this comment.
Ah, I see. I was trying to explain that I was using -2 because -1 is the one just uploaded. I'll make it clearer.
| treeBytes, err := json.Marshal(tree) | ||
| if err != nil { | ||
| log.Fatalf("Error marshalling tree created from %v: %v\n", file, err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
Is os.Exit(1) redundant since log.Fatalf logs the error and exits the program with status 1.
|
|
||
| err = os.WriteFile(filepath.Join(internal.TREE_FILES_PATH, jsonFilename), treeBytes, os.ModePerm) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error writing file %v: %v", jsonFilename, err) |
There was a problem hiding this comment.
Why not use log to log this error to be consistent
There was a problem hiding this comment.
It's just a pattern I like better to have control of exit codes. Just my preference. Can change if you want.
2 participants
No description provided.