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

ESSNTL-5487: Replace culling with deletion in api and swagger #1550

Closed

Conversation

thearifismail
Copy link
Contributor

Overview

This PR is being created to address ESSNTL-5487.
It replaces:

  1. "conventional_culling_delta" with "conventional_deletion_delta"
  2. "immutable_culling_delta" with "immutable_deletion_delta"

PR Checklist

  • Keep PR title short, ideally under 72 characters
  • Descriptive comments provided in complex code blocks
  • Tests: validate optimal/expected output
  • Tests: validate exceptions and failure scenarios
  • Tests: edge cases
  • Recovers or fails gracefully during potential resource outages (e.g. DB, Kafka)
  • Uses type hinting, if convenient
  • Documentation, if this PR changes the way other services interact with host inventory
  • Links to related PRs

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

  • Input Validation
  • Output Encoding
  • Authentication and Password Management
  • Session Management
  • Access Control
  • Cryptographic Practices
  • Error Handling and Logging
  • Data Protection
  • Communication Security
  • System Configuration
  • Database Security
  • File Management
  • Memory Management
  • General Coding Practices

@thearifismail thearifismail requested a review from a team as a code owner November 15, 2023 18:30
@fstavela
Copy link
Contributor

/retest

@@ -1669,7 +1669,7 @@ components:
type: string
format: date-time
nullable: true
culled_timestamp:
deletion_timestamp:
Copy link
Contributor

Choose a reason for hiding this comment

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

I remember that when we were discussing this change, we agreed that we will rename all of the new culling fields to "deletion", which means all the new fields introduced by custom staleness. We didn't want to rename the old fields, because it theoretically may break some customer automation, if they have any (I don't know if they do). If we want to be proper about this, we would need to release a new version of API with this kind of change.

I don't think anyone really uses this field in automation, it probably doesn't make much sense, but if there is a potential to break something, then I think all of us need to agree on it.

Copy link
Contributor Author

@thearifismail thearifismail Nov 16, 2023

Choose a reason for hiding this comment

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

hmmm... I does not make sense to create a new API version for this then. When a new API version is created, then we should sneath these in. Though the change affects a lot of files, there is no logic or functions change so it should be okay.

Copy link
Contributor

@strider strider Nov 20, 2023

Choose a reason for hiding this comment

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

hmmm, I agree with @fstavela on this. We want to avoid any potential breakage of our backwards compatibility of the API. I know that I didn't participate to this discussion, but do we have any soft deprecation rules and if yes, we should probably like to follow a deprecation period until a new API version should be released.

My two cents.

@thearifismail
Copy link
Contributor Author

This PR has been replaced by #1573

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.

3 participants