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

fix: add vra_deployment.recreate_if_expired_at attribute #514

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

azrdev
Copy link

@azrdev azrdev commented Jan 22, 2024

see #473 especially #473 (comment)

@vmwclabot vmwclabot added the dco-required DCO Required label Jan 22, 2024
@azrdev azrdev force-pushed the recreate-expired-deployments branch from 0e9fd3e to 1c6ae3a Compare April 18, 2024 07:49
@vmwclabot vmwclabot removed the dco-required DCO Required label Apr 18, 2024
@azrdev azrdev marked this pull request as ready for review April 18, 2024 07:49
@azrdev
Copy link
Author

azrdev commented Apr 18, 2024

DCO signed. before merge, please note the alternatives described in the linked issue comment

@tenthirtyam tenthirtyam changed the title add vra_deployment.recreate_if_expired_at attribute, fix #473 fix: add vra_deployment.recreate_if_expired_at attribute Jul 9, 2024
@tenthirtyam tenthirtyam self-requested a review July 9, 2024 23:54
@tenthirtyam tenthirtyam added the bug Bug label Jul 10, 2024
@azrdev
Copy link
Author

azrdev commented Aug 1, 2024

What is missing for this to be merged (or #473 to be fixed some other way)? Can I do something?

Edit: @frodenas would you be able to review & merge my patch?

@tenthirtyam tenthirtyam assigned frodenas and unassigned frodenas Aug 1, 2024
@vmware vmware deleted a comment from vmwclabot Aug 7, 2024
@azrdev azrdev force-pushed the recreate-expired-deployments branch from 1c6ae3a to 6f5fdf7 Compare November 15, 2024 10:52
@azrdev
Copy link
Author

azrdev commented Nov 15, 2024

Rebased to current main due to merge conflict. @tenthirtyam you assigned me, but I have no permissions to merge here / need a review by some maintainer. Who's able to do that?

Copy link
Collaborator

@tenthirtyam tenthirtyam left a comment

Choose a reason for hiding this comment

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

Suggesting a change as follows that would be a bit more simplified and idiomatic.

CustomizeDiff: customdiff.ForceNewIf(
    "recreate_if_expired_at",
    func(ctx context.Context, d *schema.ResourceDiff, meta interface{}) bool {
        planTime, valid := getParsedTime(d, "recreate_if_expired_at")
        if !valid {
            return false
        }
        expirationTime, valid := getParsedTime(d, "lease_expire_at")
        if !valid {
            return false
        }
        return expirationTime.Before(planTime)
    }
),

and

func getParsedTime(d *schema.ResourceDiff, key string) (time.Time, bool) {
    timeStr, ok := d.GetOk(key)
    if !ok {
        log.Printf("[INFO] Missing or invalid time string for key %s: %#v", key, timeStr)
        return time.Time{}, false
    }
    parsedTime, err := strfmt.ParseDateTime(timeStr.(string))
    if err != nil {
        log.Printf("[INFO] Failed to parse DateTime for key %s: %#v", key, timeStr)
        return time.Time{}, false
    }
    return time.Time(parsedTime), true
}

@tenthirtyam tenthirtyam added this to the .next milestone Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants