-
Notifications
You must be signed in to change notification settings - Fork 52
refactor!: remove deprecated resources #667
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m just focusing on the ECS Fargate part, since I don’t have context on the other features.
The orchestrator only hit EOL about a month ago, so my concern is that some customers might still be using it.
This provider is not Fargate specific and delivers a lot of other features. Customers might upgrade it for various reasons, but if they’re still using the orchestrator, doing so could break their ability to properly deploy the serverless agent.
My suggestion is to first confirm that all customers have moved away from the orchestrator, and only then apply this change.
@pgcrooks-sysdig We should have metrics for this
Thank you for the feedback folks. I really appreciate it, because you have more context on those resources. I saw the tests in #666 failing due to API endpoints being removed from the deprecated vulnerability resources, so removing those resources was the way to go, since they can't be used anymore. Now that we are going to have a breaking change removing resources, I wanted to remove everything that was deprecated for v2, so we don't need to v3 it to remove even more resources. Those fields were marked as deprecated 3 months ago in #643, @francesco-racciatti my suggestion for those customers is to still use v1 until they migrate to the new fields. The breaking change should not affect customers specifying proper versioning, and they can pin to a specific version if needed.
There are workarounds for that as well, like having different states for agentless deployment. Another solution may be continue the maintenance of We can as well release a WDYT? |
I completely agree with you, but still I’d prefer to stick with those deprecated parameters until we verify no one is still relying on the orchestrator, avoiding any potential impact on our customers. Please correct me if I’m wrong, but the Fargate orchestrator parameters don’t cause any failures in #666. For example, we could separate the Fargate-related changes into another PR and hold off on merging it until we confirm that no orchestrators are still in use. I've already asked Product to check. Are we in a hurry? |
Just a side note: I’d strongly avoid maintaining a separate branch for only a few optional and deprecated parameters. If we’re not in a rush, let’s wait for Product and then decide based on the data we gather. |
Agree, we could do that. However, I need to get #666 merged, and we need to remove the deprecated resources that are currently failing. I am OK with going with The other approach is just asking them to not update and still use |
When is the |
As soon as possible. We need to release #666 and remove the failing resources. |
Data confirms that there are a bunch of orchestrators in use. So I’d suggest (1) keeping the parameters instead of (2) requiring customers to stick with Sysdig Provider v1 because, from an operational standpoint, option (1) is the safest path for them. Aside
Is a major release really needed just to remove the two orchestrator parameters? They are already marked as optional. We could just drop them when migration is complete. |
Yes, even tho they are optional and may not be used, they may be part of some plan that is not active in a particular moment. Removing them without increasing the major version, will break the flow even for |
I am going to revert the changes made to the ECS Fargate resource and force a push. |
Replaces the deprecated 'trigger_after_minutes' field with its correct counterparts ('duration_seconds' or 'range_seconds') across multiple alert test files. This change aligns the tests with the recent removal of the deprecated field, ensuring that the test suite passes and reflects the current resource schemas.
3f8e57e
to
4a1a07d
Compare
Superseded by the last commits that factored out the fargate-related changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Apart from the no-op change in sysdig/data_source_sysdig_fargate_ECS_test.go
this looks safe from a ECS Fargate PoV. I cannot comment on changes beyond this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Verified that deprecated secure runtime policy can be removed in v2 of the provider.
This PR introduces a series of breaking changes to modernize the provider, removing long-deprecated resources and aligning with the latest Sysdig API capabilities. This cleanup is a preparatory step for a new major version release.
The main changes include removing old alert and policy resources and replacing them with their
v2
counterparts, as well as updating documentation and development scripts.BREAKING CHANGES:
The following resources have been removed. Users must migrate their configurations and state to the new resources.
Monitor Alerts (
sysdig_monitor_alert_*
)sysdig_monitor_alert_metric
sysdig_monitor_alert_anomaly
sysdig_monitor_alert_promql
sysdig_monitor_alert_group_outlier
sysdig_monitor_alert_event
sysdig_monitor_alert_downtime
sysdig_monitor_alert_v2_*
resources (e.g.,sysdig_monitor_alert_v2_metric
,sysdig_monitor_alert_v2_prometheus
,sysdig_monitor_alert_v2_downtime
, etc.).Secure Policy (
sysdig_secure_policy
)sysdig_secure_policy
sysdig_secure_*_policy
.Secure Notification Channel Data Source
data "sysdig_secure_notification_channel"
data "sysdig_secure_notification_channel_slack"
,data "sysdig_secure_notification_channel_email"
).Vulnerability Exception Resources
sysdig_secure_vulnerability_exception
sysdig_secure_vulnerability_exception_list
sysdig_secure_vulnerability_accept_risk
resource. This uses the new scanner.Secure Scanning Policies
sysdig_secure_scanning_policy
sysdig_secure_vulnerability_policy
resource. This uses the new scanner.Other Changes:
README.md
has been updated with modern development instructions (removing outdatedGOPATH
references).docs/index.md
has been removed to point users to the official, up-to-date documentation, preventing stale content.GNUmakefile
is updated to pass build tags to the linter, improving the development workflow.trigger_after_minutes
in alerts, to align with the latest API contracts.