Skip to content

chore: add agentapi tests #11269

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

Merged
merged 7 commits into from
Jan 26, 2024
Merged

chore: add agentapi tests #11269

merged 7 commits into from
Jan 26, 2024

Conversation

deansheather
Copy link
Member

@deansheather deansheather commented Dec 18, 2023

The tests added in this PR can be reviewed now as I probably won't be changing them (other than review comments). I'll be adding more tests for the files TODOd below in upcoming commits and I'll re-request review.

In most cases the new tests have more cases/coverage than the old ones which is nice. Some of them are quite verbose though, as I tried to maximize usage of fields on types and enums (to get conversion coverage).

TODOs:

  • metadata.go
  • stats.go
  • tailnet.go

@deansheather deansheather self-assigned this Dec 18, 2023
@deansheather deansheather marked this pull request as draft December 18, 2023 18:16
@deansheather deansheather marked this pull request as ready for review December 19, 2023 12:12
Copy link
Member Author

deansheather commented Dec 19, 2023

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @deansheather and the rest of your teammates on Graphite Graphite

Copy link
Contributor

@spikecurtis spikecurtis left a comment

Choose a reason for hiding this comment

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

I didn't finish this review, but I'm sending the comments I have since you're in a different timezone

},
}
}
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should break the manifest up into separate API calls. It's kind of a dumping ground for lots of different, unrelated kinds of information...

Not that I'm expecting it in this PR.

now, err := time.Parse("2006-01-02 15:04:05 -0700 MST", "2023-12-19 07:30:00 +1100 AEDT")
require.NoError(t, err)
now = dbtime.Time(now)
nextAutostart := now.Add(30 * time.Minute).UTC() // always sent to DB as UTC
Copy link
Contributor

Choose a reason for hiding this comment

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

to me, this smells like we need to refactor. The AgentAPI should not be in the business of computing autostart times. There should be a scheduler component that gets an activity indication from the Agent API and it computes and decides how to update the autostart / autostop schedule.

Another one for some other PR...

@github-actions github-actions bot added the stale This issue is like stale bread. label Jan 2, 2024
@matifali matifali removed the stale This issue is like stale bread. label Jan 2, 2024
@github-actions github-actions bot added stale This issue is like stale bread. and removed stale This issue is like stale bread. labels Jan 10, 2024
@github-actions github-actions bot added the stale This issue is like stale bread. label Jan 21, 2024
@github-actions github-actions bot closed this Jan 25, 2024
@spikecurtis spikecurtis reopened this Jan 25, 2024
@deansheather deansheather removed the stale This issue is like stale bread. label Jan 25, 2024
@deansheather deansheather enabled auto-merge (squash) January 26, 2024 06:52
@deansheather deansheather merged commit 2970709 into main Jan 26, 2024
@deansheather deansheather deleted the dean/agent-api-tests branch January 26, 2024 07:04
@github-actions github-actions bot locked and limited conversation to collaborators Jan 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants