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.

Sign up for GitHub

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

Jump to bottom

Print step preparation errors #816

Merged
merged 9 commits into from
Oct 6, 2022
Merged

Print step preparation errors #816

merged 9 commits into from
Oct 6, 2022

Conversation

godrei
Copy link
Contributor

@godrei godrei commented Sep 30, 2022 •
edited
Loading

Checklist

Version

Requires a MINOR version update

Context

While Bitrise CLI is running a given workflow and activating a private step, potential activation errors are not logged into the build log, which makes it hard to debug issues.

Example output when there is a typo (missing trailing e in bitrise-step-simple-git-clone-privat) in the private step URL:

Note: if the step's repository is an open source one,
you should probably use a "https://..." git clone URL,
instead of the "git@..." git clone URL which usually requires authentication
even if the repository is open source!
+------------------------------------------------------------------------------+
| (0) git::[email protected]:godrei/bitrise-step-simple-git-clone-privat.git@main |
+------------------------------------------------------------------------------+
| id: [email protected]:godrei/bitrise-step-simple-git-clone-privat.git           |
| version: main                                                                |
| collection: git                                                              |
| toolkit: bash                                                                |
| time: 2022-09-30T15:01:59+02:00                                              |
+------------------------------------------------------------------------------+
|                                                                              |
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | git::[email protected]:godrei/bitrise-step-simp... (exit code: 1)| 1.30 sec |
+---+---------------------------------------------------------------+----------+
| Issue tracker: Not provided                                                  |
| Source: Not provided                                                         |
+---+---------------------------------------------------------------+----------+

This PR updates Bitrise CLI to print step preparation errors for better debuggability:

WARN[15:04:40] Note: if the step's repository is an open source one,
you should probably use a "https://..." git clone URL,
instead of the "git@..." git clone URL which usually requires authentication
even if the repository is open source! 

ERRO[15:04:40] Preparing Step (git::[email protected]:godrei/bitrise-step-simple-git-clone-privat.git@main) failed: command failed with exit status 128 (git "clone" "--recursive" "--branch" "main" "[email protected]:godrei/bitrise-step-simple-git-clone-privat.git" "."): Cloning into '.'...
ERROR: Repository not found.
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists. 
+------------------------------------------------------------------------------+
| (0) git::[email protected]:godrei/bitrise-step-simple-git-clone-privat.git@main |
+------------------------------------------------------------------------------+
| id: [email protected]:godrei/bitrise-step-simple-git-clone-privat.git           |
| version: main                                                                |
| collection: git                                                              |
| toolkit: bash                                                                |
| time: 2022-09-30T15:04:40+02:00                                              |
+------------------------------------------------------------------------------+
|                                                                              |
|                                                                              |
+---+---------------------------------------------------------------+----------+
| x | git::[email protected]:godrei/bitrise-step-simp... (exit code: 1)| 1.38 sec |
+---+---------------------------------------------------------------+----------+
| Issue tracker: Not provided                                                  |
| Source: Not provided                                                         |
+---+---------------------------------------------------------------+----------+

Changes

  • Move inline defined registerStepRunResults func logic to BuildRunResultRegisterer.RegisterStepRunResults
    • bitrise.PrintRunningStepHeader is always called from this function
    • print step preparation errors before the step header box
  • Move step activation logic to StepActivator.ActivateStep

godrei
godrei commented Sep 30, 2022
View reviewed changes
cli/run_util.go Outdated
@@ -912,33 +704,32 @@ func activateAndRunSteps(

//
// Run step
bitrise.PrintRunningStepHeader(stepInfoPtr, mergedStep, idx)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to BuildRunResultRegisterer.RegisterStepRunResults

@godrei godrei marked this pull request as ready for review September 30, 2022 13:48
ofalvai
ofalvai reviewed Oct 3, 2022
View reviewed changes
cli/step_activator.go Outdated
log "github.com/sirupsen/logrus"
)

type StepActivator struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: everything in this file could be private if I'm not mistaken

ofalvai
ofalvai reviewed Oct 3, 2022
View reviewed changes
cli/build_run_result_registerer.go Outdated
"github.com/bitrise-io/bitrise/tools/timeoutcmd"
"github.com/bitrise-io/go-utils/colorstring"
"github.com/bitrise-io/go-utils/pointers"
coreanalytics "github.com/bitrise-io/go-utils/v2/analytics"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to import this as-is and create an alias for the v1 analytics. This way, when we get rid of the v1 imports, we won't have to remove the v2 import alias and update every affected line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not the v1 and v2 versions of the go-utils/analytics package,
but v2 go-utils/analytics and bitrise/analytics packages.

ofalvai
ofalvai reviewed Oct 3, 2022
View reviewed changes
cli/build_run_result_registerer.go Outdated
log "github.com/sirupsen/logrus"
)

type BuildRunResultRegisterer struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the correct word for this is Registrar, but I'd also be happy with something like BuildRunResultCollector

ofalvai
ofalvai previously approved these changes Oct 3, 2022
View reviewed changes
@lpusok
Copy link
Contributor

lpusok commented Oct 4, 2022 •
edited
Loading

NOTE: discussed that in case of step preparation failure change
| x | git::[email protected]:godrei/bitrise-step-simp... (exit code: 1)| 1.38 sec | to
| x | git::[email protected]:godrei/bitrise-ste... (preparation failed)| 1.38 sec | or similar.
This will be done in a followup PR.

@godrei godrei merged commit 5554029 into master Oct 6, 2022
@godrei godrei deleted the print-step-preparation-errors branch October 6, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpusok lpusok lpusok approved these changes

@ofalvai ofalvai ofalvai left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@godrei @lpusok @ofalvai