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

Add showTrend to Question #173

Merged

Conversation

MartinodF
Copy link
Contributor

This PR exposes the showTrend field on the Question object, allowing terraform users to read and update its value.
I've added basic tests for reading it (no update needed to cassettes, as they already contained it) and for validating it as a proper boolean.

While re-generating the GraphQL client and related docs to add showTrend, a couple of additional fields and values were also updated:

  • questionName was removed from Rule
  • trigger_on_new_only was added to Rule
  • FIFTEEN_MINUTES was added as an acceptable value for SchedulerPollingInterval

Those changes are in a separate commit in case you want to exclude them.

@MartinodF MartinodF requested a review from a team June 7, 2023 21:58
Copy link
Contributor

@chrichts chrichts left a comment

Choose a reason for hiding this comment

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

@MartinodF - first off thank you for making these changes and sorry for taking so long to get back to you on this. If you have time, can you make the changes required, otherwise I can close this PR and resubmit a new PR with the required changes.

I've left 2 comments. To your point, lets exclude the PR making changes to the rule resource. This is subject to change again very soon. Besides the comments in the code, the following two functions also need to be updated (both in resource_question.go): BuildQuestion and BuildCreateQuestionInput. We need to add the ShowTrend variable to the question input. So the changes should look like:

func (qm *QuestionModel) BuildQuestion() client.QuestionUpdate {
	q := client.QuestionUpdate{
		Title:           qm.Title.ValueString(),
		Description:     qm.Description.ValueString(),
		Tags:            qm.Tags,
		ShowTrend:       qm.ShowTrend.ValueBool(),
		PollingInterval: client.SchedulerPollingInterval(qm.PollingInterval.ValueString()),
	}

and

func (qm *QuestionModel) BuildCreateQuestionInput() client.CreateQuestionInput {
	q := client.CreateQuestionInput{
		Title:           qm.Title.ValueString(),
		Description:     qm.Description.ValueString(),
		PollingInterval: client.SchedulerPollingInterval(qm.PollingInterval.ValueString()),
		ShowTrend:       qm.ShowTrend.ValueBool(),
		Tags:            qm.Tags,
	}

Let me know if you have any questions.

docs/resources/question.md Outdated Show resolved Hide resolved
jupiterone/internal/client/generated.go Outdated Show resolved Hide resolved
@MartinodF MartinodF force-pushed the MartinodF/add-show-trend-to-question branch from 903214c to 65cc612 Compare August 2, 2023 20:00
@MartinodF
Copy link
Contributor Author

MartinodF commented Aug 2, 2023

Hey Chad, thank you for the feedback.

I've applied the requested changes:

  • Removed changes related to the Rule resource
  • Replaced "daily trend data" with "trend data"
  • Provided ShowTrend value in both CreateQuestionInput and QuestionUpdate
  • (Rebased onto main for good measure)

Let me know if there's anything else that needs to be addressed!

@chrichts
Copy link
Contributor

chrichts commented Aug 2, 2023

Looks good thanks @MartinodF, one last thing and then I can approve, can you run a make fmt there is an extra white space in generated.go.

Expose the showTrend field to allow terraform users to read and
update its value. Add tests for reading it (no update needed to
cassettes, as they already contained it) and for validating it as
a proper boolean.
@MartinodF MartinodF force-pushed the MartinodF/add-show-trend-to-question branch from 65cc612 to a4e8453 Compare August 2, 2023 21:39
@MartinodF
Copy link
Contributor Author

You are right. I did run make fmt, then didn't add the whitespace change to the commit because it seemed unrelated and I couldn't figure out it was the formatter removing an extra space. Done 🙂

@chrichts chrichts merged commit 4bb3be9 into JupiterOne:main Aug 3, 2023
5 checks passed
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