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

Update bitrise-init #148

Merged
merged 5 commits into from
Sep 1, 2023
Merged

Update bitrise-init #148

merged 5 commits into from
Sep 1, 2023

Conversation

godrei
Copy link
Contributor

@godrei godrei commented Aug 31, 2023

Checklist

  • I've read and followed the Contribution Guidelines
  • step.yml and README.md is updated with the changes (if needed)

Version

Requires a MAJOR version update

Context

This PR updates bitrise-init, which brings new Xcode scheme listing logic to the project scanner.

Changes

  • update bitrise-init
  • support writing scan results to a local file

@godrei godrei marked this pull request as ready for review September 1, 2023 08:04
@godrei godrei requested a review from tothszabi September 1, 2023 08:32
@@ -10,6 +10,17 @@ app:
workflows:
# ----------------------------------------------------------------
# --- workflow to Step Test
sample:
envs:
- PROJECT_DIR: "<local_project_dir>"
Copy link
Contributor

Choose a reason for hiding this comment

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

This <local_project_dir> is just a placeholder and the developer need to replace it, right? If it is then can you add a comment highlighting this?

(Because I had to look at this too long to realise that this is most probably a placeholder 😂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in bdcb0da

main.go Outdated
if err := resultClient.uploadResults(result); err != nil {
failf("Could not send back results: %s", err)
var resultBytes []byte
shouldStoreResult := resultClient != nil || strings.HasPrefix(cfg.ResultSubmitURL, "path::")
Copy link
Contributor

Choose a reason for hiding this comment

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

I like how you extracted this into a named boolean variable which helps readability a lot. Can you do the same with the path prefix check because it is also used at 4 different places and if we name it then it will be much faster to understand what it means. Something like

isLocalSave := strings.HasPrefix(cfg.ResultSubmitURL, "path::")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in bdcb0da

main.go Outdated
log.TInfof("Submitting results...")
if err := resultClient.uploadResults(result); err != nil {
failf("Could not send back results: %s", err)
var resultBytes []byte
Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is not used outside of the if statement below so I think it can be moved from here to there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in bdcb0da

main.go Outdated
var resultBytes []byte
shouldStoreResult := resultClient != nil || strings.HasPrefix(cfg.ResultSubmitURL, "path::")
if shouldStoreResult {
resultBytes, err = json.MarshalIndent(result, "", "\t")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it is only whitespace but this will change the format of the result a bit. Just double checking that whatever component relies on this result in our system can still read it, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the current format: https://github.com/bitrise-steplib/steps-project-scanner/blob/master/submitresult.go#L91
I didn't plan to change it with this PR.

submitresult.go Outdated

func (c *resultClient) uploadResults( /*result models.ScanResultModel, */ bytes []byte) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this commented out parameter can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in bdcb0da

main.go Outdated
}

log.TDonef("Results submitted.")
} else if strings.HasPrefix(cfg.ResultSubmitURL, "path::") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It was not clear at first what this new local save feature means. Can you add some explanation that this is not used in prod and it is mostly for local testing with the sample workflow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in bdcb0da

@godrei godrei merged commit 8ebfb6c into master Sep 1, 2023
@godrei godrei deleted the update-bitrise-init branch September 1, 2023 10:57
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.

2 participants