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

Remove internal fields from monitoring uptime check #1974

Merged
merged 2 commits into from
Jun 25, 2019

Conversation

rileykarson
Copy link
Member

@rileykarson rileykarson commented Jun 25, 2019

We never tested these fields, and turns out they don't work. They were erroneously published in the API (http://b/122540324)

Release Note for Downstream PRs (will be copied)

Removed non-existent fields `is_internal` and `internal_checkers` from `google_monitoring_uptime_check_config`

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
This PR seems not to have generated downstream PRs before, as of 2e5236e.

Pull request statuses

No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I built this PR into one or more new PRs on other repositories, and when those are closed, this PR will also be merged and closed.
depends: hashicorp/terraform-provider-google-beta#888
depends: GoogleCloudPlatform/terraform-google-conversion#118
depends: hashicorp/terraform-provider-google#3919

Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

I think it's better to add deprecated to the fields in the API.yaml. As of right now the rest API docs and the discovery docs still have them listed. I think with the addition of tooling around generating the api.yaml and/or linting it I would prefer to leave them in the file rather than adding custom code to Terraform to remove them.

@rileykarson
Copy link
Member Author

rileykarson commented Jun 25, 2019

I disagree- these fields never existed outside of the internal/ possibly the EAP API and were never intended to be surfaced to users. If a user ever specified them, they never worked. The deprecated message passed over in the REST reference does not refer to the fields being deprecated at GA, it refers to them being deprecated in a private API version. Also isInternal was hard-removed in the API.

If/when we generate API definitions, we shouldn't include deprecated fields.

@chrisst
Copy link
Contributor

chrisst commented Jun 25, 2019

Ideal scenario is definitely not to add deprecated fields to the api.yaml in the first place. But right now there is no way to identify from the discovery docs that these fields are deprecated, so any tooling in the future will lose the context and potentially re-add them. Adding a deprecation flag in the yaml is effectively what the UI has done with the 'warning' in the rest documentation.

I think it's a reasonable pattern to include a deprecation flag for fields that were once part of an API and now are no longer.

@rileykarson
Copy link
Member Author

These were never part of the API though, and were erroneously published as part of the REST reference.

I think we should handle deprecated fields with exclude: true, and not a new field. Since we disagree, let's leave solving how we handle that to the actual implementation of a definitions generator.

Copy link
Contributor

@chrisst chrisst left a comment

Choose a reason for hiding this comment

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

fair enough

@modular-magician
Copy link
Collaborator

Hi! I'm the modular magician, I work on Magic Modules.
I see that this PR has already had some downstream PRs generated. Any open downstreams are already updated to your most recent commit, 2e5236e.

Pull request statuses

terraform-provider-google-beta already has an open PR.
terraform-google-conversion already has an open PR.
terraform-provider-google already has an open PR.
No diff detected in Ansible.
No diff detected in Inspec.

New Pull Requests

I didn't open any new pull requests because of this PR.

rileykarson and others added 2 commits June 25, 2019 21:26
Tracked submodules are build/terraform-beta build/terraform-mapper build/terraform build/ansible build/inspec.
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.

4 participants