Skip to content

[CDTOOL-1650] Add support for account and workspace level time series commands#1823

Open
rcaril wants to merge 5 commits into
mainfrom
rcaril/CDTOOL-1650-add-support-for-time-series
Open

[CDTOOL-1650] Add support for account and workspace level time series commands#1823
rcaril wants to merge 5 commits into
mainfrom
rcaril/CDTOOL-1650-add-support-for-time-series

Conversation

@rcaril

@rcaril rcaril commented Jun 16, 2026

Copy link
Copy Markdown
Member

Change summary

This PR adds support for NGWAF Time Series commands.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  • Does your submission pass tests?
make test TEST_ARGS="-run TestWorkspaceTimeseriesGet ./pkg/commands/ngwaf/workspace/timeseries"
ok      github.com/fastly/cli/pkg/commands/ngwaf/workspace/timeseries   1.102s

make test TEST_ARGS="-run TestTimeseriesList ./pkg/commands/ngwaf/timeseries"
ok      github.com/fastly/cli/pkg/commands/ngwaf/timeseries     1.140s

Changes to Core Features:

  • Have you written new tests for your core changes, as applicable?
  • Have you successfully run tests with your changes locally?

Notes:

This PR is pending the release of this go-fastly feature.

@rcaril rcaril marked this pull request as ready for review June 22, 2026 14:32
@rcaril rcaril requested a review from a team as a code owner June 22, 2026 14:32
@rcaril rcaril requested a review from philippschulte June 22, 2026 14:32
@rcaril

rcaril commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

Tests will fail until fastly/go-fastly#828 is released and the repo is bumped accordingly.

@philippschulte philippschulte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The overall CLI wiring looks good, but I think we should add a non-empty workspace time-series test before merging.

This PR does not actually exercise the new data shape or the table renderer for real workspace time-series rows.

Can we add a non-empty workspace time-series CLI test?

@rcaril

rcaril commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

The overall CLI wiring looks good, but I think we should add a non-empty workspace time-series test before merging.

This PR does not actually exercise the new data shape or the table renderer for real workspace time-series rows.

Can we add a non-empty workspace time-series CLI test?

I tried various ways to get data to populate on a service we have that is streaming requests - but was unsuccessful. Let me see if I can do anything else to get data examples.

@rcaril

rcaril commented Jun 22, 2026

Copy link
Copy Markdown
Member Author

The overall CLI wiring looks good, but I think we should add a non-empty workspace time-series test before merging.

This PR does not actually exercise the new data shape or the table renderer for real workspace time-series rows.

Can we add a non-empty workspace time-series CLI test?

Added in ded7d2d.

 make test TEST_ARGS="-run TestWorkspaceTimeseriesGet ./pkg/commands/ngwaf/workspace/timeseries"
ok      github.com/fastly/cli/pkg/commands/ngwaf/workspace/timeseries   1.130s

@rcaril rcaril requested a review from philippschulte June 22, 2026 18:59

@philippschulte philippschulte left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks, this addresses my concern.

Let me know if you need another approval after addressing the failed linting and test jobs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants