Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add resources_monitoring field to agent#331

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
defelmnq merged 12 commits intomainfromadd-monitors
Feb 7, 2025
Merged

Conversation

defelmnq
Copy link
Contributor

As required perthis issue - we need to add a new field in terraform-provider-coder , more specifically in the agent.

Copy link
Contributor

@DanielleMaywoodDanielleMaywood left a comment

Choose a reason for hiding this comment

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

LGTM:shipit:

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

Blocking this from accidental merge since we already have one approval, but some work is still required for validation.

@defelmnq
Copy link
ContributorAuthor

@mtojek@dannykopping - I improved the validation and added test cases for all the main cases, should be way better now. ✅

@defelmnq
Copy link
ContributorAuthor

I added some more cases to ensure we test all new fields and options.

Just a point remaining to discuss based on what we want to do around the duplicates validation. More details on this comment but globally I copy pasted metadata - still if there's any better place I am open to change it.

Otherwise I tested the tf-provider with coder/coder and it works exactly as we want - so I'm happy about the current behavior.

@matifali
Copy link
Member

matifali commentedFeb 7, 2025
edited
Loading

I have two suggestions.

  1. Can we add an error that provider emits if someone tries to useresource_monitoring with Coder version 2.19 and below? The Error should tell the user what minimum Coder version the provider needs for this feature to work.
  2. Tag the upcoming release asv2.2.0. We can also usev2.2.0-rc.0 until we release a compatible Coder release and then move tov2.2.0.

    One downside of an RC release is that if we fix anything else, we need to do a patch frommain that will include this new feature. I also do not want us to release from branches.

WDYT@johnstcn as we recently spoke about versions for our provider?

defelmnq reacted with eyes emoji

@defelmnq
Copy link
ContributorAuthor

I'll indeed defer answers to your questions to@johnstcn - not sure if this is doable and how , but happy to implement the check if we want to.

About the versioning, both are fine to me. I already tested it a lot with the Coder code we will deploy but a rc can be good too.

Copy link
Contributor

@dannykoppingdannykopping left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Member

@mtojekmtojek left a comment

Choose a reason for hiding this comment

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

Nice!

@johnstcn
Copy link
Member

I have two suggestions.

  1. Can we add an error that provider emits if someone tries to useresource_monitoring with Coder version 2.19 and below? The Error should tell the user what minimum Coder version the provider needs for this feature to work.

It should be possible to hit the/api/v2/buildinfo endpoint. We can't importcodersdk here though so we'd need to do a little bit of copying. But it should definitely be possible! I think that's a good follow-up PR.

  1. Tag the upcoming release asv2.2.0. We can also usev2.2.0-rc.0 until we release a compatible Coder release and then move tov2.2.0.

    One downside of an RC release is that if we fix anything else, we need to do a patch frommain that will include this new feature. I also do not want us to release from branches.

WDYT@johnstcn as we recently spoke about versions for our provider?

I'm fine with RC releases. It's a little annoying to have to cherry-pick each time, but it's not the end of the world.

matifali reacted with thumbs up emoji

@defelmnqdefelmnq merged commit1f95807 intomainFeb 7, 2025
8 checks passed
@defelmnqdefelmnq deleted the add-monitors branchFebruary 7, 2025 13:58
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsFeb 7, 2025
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@dannykoppingdannykoppingdannykopping approved these changes

@johnstcnjohnstcnjohnstcn approved these changes

@mtojekmtojekmtojek approved these changes

@DanielleMaywoodDanielleMaywoodDanielleMaywood approved these changes

@matifalimatifaliAwaiting requested review from matifali

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

Successfully merging this pull request may close these issues.

6 participants
@defelmnq@matifali@johnstcn@dannykopping@mtojek@DanielleMaywood

[8]ページ先頭

©2009-2025 Movatter.jp