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

Cli fix pipeline logs #3913

Merged
merged 17 commits into from
Jul 18, 2024
Merged

Cli fix pipeline logs #3913

merged 17 commits into from
Jul 18, 2024

Conversation

smainz
Copy link
Contributor

@smainz smainz commented Jul 16, 2024

Fixes #3911 fixes #3912

  • Insert line break between log lines
  • Indicate pipelineis not an optional argument to woodpecker pipeline logs|ps
  • Provide better error messages for false arguments in woodpecker pipeline logs|ps
  • Add option to show logs for whole pipeline
  • Print Workflow and StepID in woodpecker-cli pipeline ps
  • Use step name or step PID instead of stepID

@smainz
Copy link
Contributor Author

smainz commented Jul 16, 2024

Need advice on how to display Workflows:

Workflows do have ID, PIDand a Name. What is the canonical way to identify a workflow to the user?
In the UI the Name is used, but are these unique?

For Steps the UI shows the name, the url uses the PID and the cli uses the ID.

Btw: What is step 1 (PID)? All my pipeline logs start with step 2.

@anbraten
Copy link
Member

anbraten commented Jul 16, 2024

Those changes look already really promising.

Workflows do have ID, PID and a Name. What is the canonical way to identify a workflow to the user?
In the UI the Name is used, but are these unique?

From a UX perspective I think the workflow name would be best to show. They are in general unique, for matrix workflow they however are not and need to be identified by the matrix parameters. Maybe adding the PID or the matrix parameters behind it like # test (arch: amd64) would be an option and steps could be listed as ## step-name (pid)

Btw: What is step 1 (PID)? All my pipeline logs start with step 2.

The PID is an incremental number which is first used for the workflows followed by the steps. So PID: 1 is assigned to your first workflow. Has to do with the past that workflows where stored as steps in the database.

Having to use the stepID as argument is in general pretty strange, right? Maybe changing it to PID or name would be better. The stepID can be simply retrieved from the api using /api/repos/<repo-id>/pipelines/<pipeline-id>

@@ -41,7 +48,7 @@ func pipelinePs(c *cli.Context) error {
}
repoID, err := internal.ParseRepo(client, repoIDOrFullName)
if err != nil {
return err
return fmt.Errorf("invalid repo-id or repo-full-name: '%s'", repoIDOrFullName)
Copy link
Member

Choose a reason for hiding this comment

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

Could you test if the changes from #3830 improve it for you already. They were not released yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it does not, because I do use the woodpecker-cli from outside a project directory / git directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should wrap error from ParseRepo, at least.
cli/internal/util.go:122

Does could not get repository fall into invalid repo-id or repo-full-name category?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the right way to do it?
My go skills are at a beginners level if at all.

@anbraten anbraten marked this pull request as draft July 16, 2024 08:29
@anbraten anbraten added enhancement improve existing features cli labels Jul 16, 2024
@smainz
Copy link
Contributor Author

smainz commented Jul 16, 2024

@anbraten Markdown ate your proposal for showing steps.

stepID is only in logs and log_purge, I can try to rename that. Same or different PR?

@zc-devs
Copy link
Contributor

zc-devs commented Jul 16, 2024

server/model/step.go:28

	ID         int64       `json:"id"                   xorm:"pk autoincr 'id'"`
	UUID       string      `json:"uuid"                 xorm:"INDEX 'uuid'"`
	PipelineID int64       `json:"pipeline_id"          xorm:"UNIQUE(s) INDEX 'pipeline_id'"`
	PID        int         `json:"pid"                  xorm:"UNIQUE(s) 'pid'"`
	PPID       int         `json:"ppid"                 xorm:"ppid"`

We need more gold IDs, epecially with P prefix.


AFAIK, in logs API pipeline number is used.

@anbraten
Copy link
Member

anbraten commented Jul 16, 2024

We need more gold IDs, epecially with P prefix.

Absolutely 😅 . Those PID and PPID drive me crazy each time I have to check what they are used for. PPID could actually be renamed to workflow_id, pid could be removed from workflows in general, step uuid is kinda a hack for some compiler related stuff 6543 has added.

@zc-devs
Copy link
Contributor

zc-devs commented Jul 16, 2024

the matrix parameters

as more variables we have, as difficult it becomes.

@woodpecker-bot
Copy link
Collaborator

woodpecker-bot commented Jul 16, 2024

Deployment of preview was torn down

@smainz smainz changed the title WIP: Cli fix pipeline logs Cli fix pipeline logs Jul 17, 2024
@smainz
Copy link
Contributor Author

smainz commented Jul 17, 2024

Should be ready now for review.
Once merged, I will change woodpecker-cli log purge from stepID to step name / step PID.

@anbraten anbraten marked this pull request as ready for review July 17, 2024 09:06
cli/pipeline/ps.go Outdated Show resolved Hide resolved
Co-authored-by: Thomas Anderson <127358482+zc-devs@users.noreply.github.com>
cli/internal/util.go Outdated Show resolved Hide resolved
cli/pipeline/ps.go Outdated Show resolved Hide resolved
@qwerty287
Copy link
Contributor

Actually, I'm wondering now: Is it still possible to use the ID?

If not this change is breaking.

Copy link
Contributor

@qwerty287 qwerty287 left a comment

Choose a reason for hiding this comment

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

@6543
Copy link
Member

6543 commented Jul 18, 2024

as per #3913 (comment)

Does the ParseStep not exactly do that?!?

This comment was marked as off-topic.

@qwerty287
Copy link
Contributor

No, that's the problem. If you give a number/integer as argument, it will be parsed as PID, but not as ID. That's different.

@smainz
Copy link
Contributor Author

smainz commented Jul 18, 2024

Yes, it is using the PID not the ID. One may consider this breaking, but I do not see, how one would have known about the ID of a stepbefore ,as it is displayed nowhere.

If this is an issue, and you want to keep the ID, the PID could be given as #<number>, but this may get confusing for users.

@6543
Copy link
Member

6543 commented Jul 18, 2024

uh yes it is breaking ... I'd like to have this in, I'll revert some parts and this part can then be scheduled for 3.x

@6543 6543 requested a review from qwerty287 July 18, 2024 18:25
@smainz
Copy link
Contributor Author

smainz commented Jul 18, 2024

If we change it that way, pipeline logs should show the step ID, shouldn't it?

@6543 6543 merged commit 49c2029 into woodpecker-ci:main Jul 18, 2024
6 of 7 checks passed
@zc-devs
Copy link
Contributor

zc-devs commented Jul 18, 2024

how one would have known about the ID of a step?

pipeline info should display it, I guess.

https://ci.woodpecker-ci.org/swagger/index.html#/Pipelines/get_repos__repo_id__pipelines__number_

@6543
Copy link
Member

6543 commented Jul 18, 2024

-> #3927

@6543
Copy link
Member

6543 commented Jul 18, 2024

thanks @smainz & @qwerty287 :)

@woodpecker-bot woodpecker-bot mentioned this pull request Jul 18, 2024
1 task
6543 pushed a commit that referenced this pull request Jul 18, 2024
## [2.7.0](https://github.com/woodpecker-ci/woodpecker/releases/tag/v2.7.0) - 2024-07-18

### 🔒 Security

- Add blocklist of environment variables who could alter execution of plugins [[#3934](#3934)]
- Make sure plugins only mount the workspace base in a predefinde location [[#3933](#3933)]
- Disallow to set arbitrary environments for plugins [[#3909](#3909)]
- Use proper oauth state [[#3847](#3847)]
- Enhance token checking [[#3842](#3842)]
- Bump github.com/hashicorp/go-retryablehttp v0.7.5 -> v0.7.7 [[#3834](#3834)]

### ✨ Features

- Gracefully shutdown server [[#3896](#3896)]
- Gracefully shutdown agent [[#3895](#3895)]
- Convert urls in logs to links  [[#3904](#3904)]
- Allow login using multiple forges [[#3822](#3822)]
- Global and organization registries [[#1672](#1672)]
- Cli get repo from git remote [[#3830](#3830)]
- Add api for forges [[#3733](#3733)]

### 📈 Enhancement

- Cli fix pipeline logs [[#3913](#3913)]
- Migrate to github.com/urfave/cli/v3 [[#2951](#2951)]
- Allow to change the working directory also for plugins and services [[#3914](#3914)]
- Remove `unplugin-icons` [[#3809](#3809)]
- Release windows binaries as zip file [[#3906](#3906)]
- Convert to openapi 3.0 [[#3897](#3897)]
- Enhance pipeline list [[#3898](#3898)]
- Add user registries UI [[#3888](#3888)]
- Sort users by login [[#3891](#3891)]
- Exclude dummy backend in production [[#3877](#3877)]
- Fix deploy task env [[#3878](#3878)]
- Get default branch and show message in pipeline list [[#3867](#3867)]
- Add timestamp for last work done by agent [[#3844](#3844)]
- Adjust logger types [[#3859](#3859)]
- Cleanup state reporting [[#3850](#3850)]
- Unify DB tables/columns [[#3806](#3806)]
- Let webhook pass on pipeline parsing error [[#3829](#3829)]
- Exclude mocks from release build [[#3831](#3831)]
- K8s secrets reference from step [[#3655](#3655)]

### 🐛 Bug Fixes

- Handle empty repositories in gitea when listing PRs [[#3925](#3925)]
- Update alpine package dep for docker images [[#3917](#3917)]
- Don't report error if agent was terminated gracefully [[#3894](#3894)]
- Let agents continuously report their health [[#3893](#3893)]
- Ignore warnings for cli exec [[#3868](#3868)]
- Correct favicon states [[#3832](#3832)]
- Cleanup of the login flow and tests [[#3810](#3810)]
- Fix newlines in logs [[#3808](#3808)]
- Fix authentication error handling [[#3807](#3807)]

### 📚 Documentation

- Streamline docs for new users [[#3803](#3803)]
- Add mastodon verification [[#3843](#3843)]
- chore(deps): update docs npm deps non-major [[#3837](#3837)]
- fix(deps): update docs npm deps non-major [[#3824](#3824)]
- Add openSUSE package [[#3800](#3800)]
- chore(deps): update docs npm deps non-major [[#3798](#3798)]
- Add "Docker Tags" Plugin [[#3796](#3796)]
- chore(deps): update dependency marked to v13 [[#3792](#3792)]
- chore: fix some comments [[#3788](#3788)]

### Misc

- chore(deps): update web npm deps non-major [[#3930](#3930)]
- chore(deps): update dependency vitest to v2 [[#3905](#3905)]
- fix(deps): update module github.com/google/go-github/v62 to v63 [[#3910](#3910)]
- chore(deps): update docker.io/woodpeckerci/plugin-docker-buildx docker tag to v4.1.0 [[#3908](#3908)]
- Update plugin-git and add renovate trigger [[#3901](#3901)]
- chore(deps): update docker.io/mstruebing/editorconfig-checker docker tag to v3.0.3 [[#3903](#3903)]
- fix(deps): update golang-packages [[#3875](#3875)]
- chore(deps): lock file maintenance [[#3876](#3876)]
- [pre-commit.ci] pre-commit autoupdate [[#3862](#3862)]
- Add dummy backend [[#3820](#3820)]
- chore(deps): update dependency replace-in-file to v8 [[#3852](#3852)]
- Update forgejo sdk [[#3840](#3840)]
- chore(deps): lock file maintenance [[#3838](#3838)]
- Allow to set dist dir using env var [[#3814](#3814)]
- chore(deps): lock file maintenance [[#3805](#3805)]
- chore(deps): update docker.io/lycheeverse/lychee docker tag to v0.15.1 [[#3797](#3797)]
@woodpecker-bot woodpecker-bot mentioned this pull request Jul 19, 2024
1 task
6543 added a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
Co-authored-by: Thomas Anderson <127358482+zc-devs@users.noreply.github.com>
Co-authored-by: 6543 <6543@obermui.de>
6543 pushed a commit to 6543-forks/woodpecker that referenced this pull request Sep 5, 2024
## [2.7.0](https://github.com/woodpecker-ci/woodpecker/releases/tag/v2.7.0) - 2024-07-18

### 🔒 Security

- Add blocklist of environment variables who could alter execution of plugins [[woodpecker-ci#3934](woodpecker-ci#3934)]
- Make sure plugins only mount the workspace base in a predefinde location [[woodpecker-ci#3933](woodpecker-ci#3933)]
- Disallow to set arbitrary environments for plugins [[woodpecker-ci#3909](woodpecker-ci#3909)]
- Use proper oauth state [[woodpecker-ci#3847](woodpecker-ci#3847)]
- Enhance token checking [[woodpecker-ci#3842](woodpecker-ci#3842)]
- Bump github.com/hashicorp/go-retryablehttp v0.7.5 -> v0.7.7 [[woodpecker-ci#3834](woodpecker-ci#3834)]

### ✨ Features

- Gracefully shutdown server [[woodpecker-ci#3896](woodpecker-ci#3896)]
- Gracefully shutdown agent [[woodpecker-ci#3895](woodpecker-ci#3895)]
- Convert urls in logs to links  [[woodpecker-ci#3904](woodpecker-ci#3904)]
- Allow login using multiple forges [[woodpecker-ci#3822](woodpecker-ci#3822)]
- Global and organization registries [[woodpecker-ci#1672](woodpecker-ci#1672)]
- Cli get repo from git remote [[woodpecker-ci#3830](woodpecker-ci#3830)]
- Add api for forges [[woodpecker-ci#3733](woodpecker-ci#3733)]

### 📈 Enhancement

- Cli fix pipeline logs [[woodpecker-ci#3913](woodpecker-ci#3913)]
- Migrate to github.com/urfave/cli/v3 [[woodpecker-ci#2951](woodpecker-ci#2951)]
- Allow to change the working directory also for plugins and services [[woodpecker-ci#3914](woodpecker-ci#3914)]
- Remove `unplugin-icons` [[woodpecker-ci#3809](woodpecker-ci#3809)]
- Release windows binaries as zip file [[woodpecker-ci#3906](woodpecker-ci#3906)]
- Convert to openapi 3.0 [[woodpecker-ci#3897](woodpecker-ci#3897)]
- Enhance pipeline list [[woodpecker-ci#3898](woodpecker-ci#3898)]
- Add user registries UI [[woodpecker-ci#3888](woodpecker-ci#3888)]
- Sort users by login [[woodpecker-ci#3891](woodpecker-ci#3891)]
- Exclude dummy backend in production [[woodpecker-ci#3877](woodpecker-ci#3877)]
- Fix deploy task env [[woodpecker-ci#3878](woodpecker-ci#3878)]
- Get default branch and show message in pipeline list [[woodpecker-ci#3867](woodpecker-ci#3867)]
- Add timestamp for last work done by agent [[woodpecker-ci#3844](woodpecker-ci#3844)]
- Adjust logger types [[woodpecker-ci#3859](woodpecker-ci#3859)]
- Cleanup state reporting [[woodpecker-ci#3850](woodpecker-ci#3850)]
- Unify DB tables/columns [[woodpecker-ci#3806](woodpecker-ci#3806)]
- Let webhook pass on pipeline parsing error [[woodpecker-ci#3829](woodpecker-ci#3829)]
- Exclude mocks from release build [[woodpecker-ci#3831](woodpecker-ci#3831)]
- K8s secrets reference from step [[woodpecker-ci#3655](woodpecker-ci#3655)]

### 🐛 Bug Fixes

- Handle empty repositories in gitea when listing PRs [[woodpecker-ci#3925](woodpecker-ci#3925)]
- Update alpine package dep for docker images [[woodpecker-ci#3917](woodpecker-ci#3917)]
- Don't report error if agent was terminated gracefully [[woodpecker-ci#3894](woodpecker-ci#3894)]
- Let agents continuously report their health [[woodpecker-ci#3893](woodpecker-ci#3893)]
- Ignore warnings for cli exec [[woodpecker-ci#3868](woodpecker-ci#3868)]
- Correct favicon states [[woodpecker-ci#3832](woodpecker-ci#3832)]
- Cleanup of the login flow and tests [[woodpecker-ci#3810](woodpecker-ci#3810)]
- Fix newlines in logs [[woodpecker-ci#3808](woodpecker-ci#3808)]
- Fix authentication error handling [[woodpecker-ci#3807](woodpecker-ci#3807)]

### 📚 Documentation

- Streamline docs for new users [[woodpecker-ci#3803](woodpecker-ci#3803)]
- Add mastodon verification [[woodpecker-ci#3843](woodpecker-ci#3843)]
- chore(deps): update docs npm deps non-major [[woodpecker-ci#3837](woodpecker-ci#3837)]
- fix(deps): update docs npm deps non-major [[woodpecker-ci#3824](woodpecker-ci#3824)]
- Add openSUSE package [[woodpecker-ci#3800](woodpecker-ci#3800)]
- chore(deps): update docs npm deps non-major [[woodpecker-ci#3798](woodpecker-ci#3798)]
- Add "Docker Tags" Plugin [[woodpecker-ci#3796](woodpecker-ci#3796)]
- chore(deps): update dependency marked to v13 [[woodpecker-ci#3792](woodpecker-ci#3792)]
- chore: fix some comments [[woodpecker-ci#3788](woodpecker-ci#3788)]

### Misc

- chore(deps): update web npm deps non-major [[woodpecker-ci#3930](woodpecker-ci#3930)]
- chore(deps): update dependency vitest to v2 [[woodpecker-ci#3905](woodpecker-ci#3905)]
- fix(deps): update module github.com/google/go-github/v62 to v63 [[woodpecker-ci#3910](woodpecker-ci#3910)]
- chore(deps): update docker.io/woodpeckerci/plugin-docker-buildx docker tag to v4.1.0 [[woodpecker-ci#3908](woodpecker-ci#3908)]
- Update plugin-git and add renovate trigger [[woodpecker-ci#3901](woodpecker-ci#3901)]
- chore(deps): update docker.io/mstruebing/editorconfig-checker docker tag to v3.0.3 [[woodpecker-ci#3903](woodpecker-ci#3903)]
- fix(deps): update golang-packages [[woodpecker-ci#3875](woodpecker-ci#3875)]
- chore(deps): lock file maintenance [[woodpecker-ci#3876](woodpecker-ci#3876)]
- [pre-commit.ci] pre-commit autoupdate [[woodpecker-ci#3862](woodpecker-ci#3862)]
- Add dummy backend [[woodpecker-ci#3820](woodpecker-ci#3820)]
- chore(deps): update dependency replace-in-file to v8 [[woodpecker-ci#3852](woodpecker-ci#3852)]
- Update forgejo sdk [[woodpecker-ci#3840](woodpecker-ci#3840)]
- chore(deps): lock file maintenance [[woodpecker-ci#3838](woodpecker-ci#3838)]
- Allow to set dist dir using env var [[woodpecker-ci#3814](woodpecker-ci#3814)]
- chore(deps): lock file maintenance [[woodpecker-ci#3805](woodpecker-ci#3805)]
- chore(deps): update docker.io/lycheeverse/lychee docker tag to v0.15.1 [[woodpecker-ci#3797](woodpecker-ci#3797)]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli enhancement improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

There are no line breaks in the step logs when printed by cli woodpecker-cli pipeline logs hardly usabe
6 participants