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

refactor(types)!: move worker type to server and nest API object #997

Merged
merged 18 commits into from
Mar 19, 2024

Conversation

ecrupper
Copy link
Contributor

@ecrupper ecrupper commented Nov 1, 2023

Proposal ref: go-vela/community#639

@jbrockopp saw this refactor as a good opportunity to migrate off of the go-vela/types repo a while back.

Starting with worker since it is a very low user-requested resource but is still leveraged often enough by the Vela application that we would know if something wasn't quite right very early.

I was also itching to start with worker, as the admin page would benefit greatly from clickable build links sourced from the ListWorkers API response.

As a visual:

OLD

{
    "id": 1,
    "hostname": "worker",
    "address": "http://worker:8080/",
    "routes": [
      "vela"
    ],
    "active": true,
    "status": "busy",
    "last_status_update_at": 1698852759,
    "running_builds": [
      "1"
    ],
    "last_build_started_at": 1698852759,
    "last_build_finished_at": 0,
    "last_checked_in": 1698852748,
    "build_limit": 1
  }

NEW

{
    "id": 1,
    "hostname": "worker",
    "address": "http://worker:8080/",
    "routes": [
      "vela"
    ],
    "active": true,
    "status": "busy",
    "last_status_update_at": 1698852759,
    "running_builds": [
      {
        "id": 1,
        "repo_id": 1,
        "pipeline_id": 1,
        "number": 1,
        "parent": 0,
        "event": "push",
        "event_action": "",
        "status": "running",
        "error": "",
        "enqueued": 1698852759,
        "created": 1698852759,
        "started": 1698852759,
        "finished": 0,
        "deploy": "",
        "deploy_payload": {},
        "clone": "https://github.com/Octocat/myvela.git",
        "source": "https://github.com/Octocat/myvela/commit/123abc",
        "title": "push received from https://github.com/Octocat/myvela",
        "message": "Update .vela.yml",
        "commit": "123abc",
        "sender": "Octocat",
        "author": "Octocat",
        "email": "[email protected]",
        "link": "http://localhost:8888/Octocat/myvela/1",
        "branch": "main",
        "ref": "refs/heads/main",
        "base_ref": "",
        "head_ref": "",
        "host": "worker",
        "runtime": "docker",
        "distribution": "linux"
      }
    ],
    "last_build_started_at": 1698852759,
    "last_build_finished_at": 0,
    "last_checked_in": 1698852748,
    "build_limit": 1
  }

@ecrupper ecrupper requested a review from a team as a code owner November 1, 2023 18:04
Copy link

codecov bot commented Nov 1, 2023

Codecov Report

Attention: Patch coverage is 92.11618% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 62.97%. Comparing base (f77ef48) to head (7cbf387).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #997      +/-   ##
==========================================
+ Coverage   62.64%   62.97%   +0.33%     
==========================================
  Files         345      347       +2     
  Lines       10304    10564     +260     
==========================================
+ Hits         6455     6653     +198     
- Misses       3373     3427      +54     
- Partials      476      484       +8     
Files Coverage Δ
api/types/worker.go 100.00% <100.00%> (ø)
database/worker/create.go 80.00% <100.00%> (ø)
database/worker/delete.go 100.00% <100.00%> (ø)
database/worker/get.go 81.81% <100.00%> (ø)
database/worker/get_hostname.go 84.61% <100.00%> (ø)
database/worker/list.go 89.47% <100.00%> (ø)
database/worker/update.go 80.00% <100.00%> (ø)
router/middleware/worker/context.go 100.00% <100.00%> (ø)
router/middleware/worker/worker.go 77.77% <100.00%> (ø)
util/util.go 17.85% <100.00%> (ø)
... and 2 more

database/worker/create.go Show resolved Hide resolved
database/worker/delete.go Show resolved Hide resolved
api/worker/get.go Show resolved Hide resolved
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

looks good overall. it seems like the we would have to mark this as breaking with the field name change here https://github.com/go-vela/server/pull/997/files#diff-a0e7f3f0dda9cc1df4b6b67ce3b81bdf66406453632b6777ad555838eaea080aR22 (see https://github.com/go-vela/types/blob/bab8877f46068f83fc2775f6148303be018ae248/library/worker.go#L20 for reference). in local testing, "running_builds" never showed up in the response. i believe we need accompanying changes in sdk-go and worker.

database/integration_test.go Show resolved Hide resolved
database/types/sanitize.go Outdated Show resolved Hide resolved
database/worker/worker.go Outdated Show resolved Hide resolved
@ecrupper ecrupper changed the title refactor(types): move worker type to server and nest API object refactor(types)!: move worker type to server and nest API object Mar 5, 2024
jbrockopp
jbrockopp previously approved these changes Mar 15, 2024
Copy link
Contributor

@jbrockopp jbrockopp left a comment

Choose a reason for hiding this comment

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

LGTM

plyr4
plyr4 previously approved these changes Mar 18, 2024
Copy link
Member

@wass3rw3rk wass3rw3rk left a comment

Choose a reason for hiding this comment

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

👍🏼

@ecrupper ecrupper merged commit 1809638 into main Mar 19, 2024
17 of 19 checks passed
@ecrupper ecrupper deleted the refactor/nested-api/worker branch March 19, 2024 16:11
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.

4 participants