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 deny purge property to stream spec #186

Merged
merged 2 commits into from
Jul 26, 2024
Merged

Conversation

vsinger
Copy link
Contributor

@vsinger vsinger commented Jun 13, 2024

Added possibility to set "deny_purge" property on a stream

@@ -39,6 +39,7 @@ type sharedInformerFactory struct {
lock sync.Mutex
defaultResync time.Duration
customResync map[reflect.Type]time.Duration
transform cache.TransformFunc
Copy link
Member

Choose a reason for hiding this comment

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

@vsinger The "deny purge" specific changes look good. Can you share what this addition does and how it is used below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bruth this has been generated after the local build. I think the files have not been generated since the latest dependency updates. The transform func has been added to kubernetes/client-go a while ago, see here: kubernetes/client-go@411a118

Copy link
Member

Choose a reason for hiding this comment

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

Understood thanks!

@bruth bruth requested a review from samuelattwood June 13, 2024 10:55
@vsinger
Copy link
Contributor Author

vsinger commented Jun 24, 2024

Hey @bruth @samuelattwood,
any chance to get this merged in the near future? Is there anything open from my side?

@Jarema
Copy link
Member

Jarema commented Jun 24, 2024

I think we should be good to merge this.
@bruth any objection?

@vsinger
Copy link
Contributor Author

vsinger commented Jul 9, 2024

Any updates on this? @bruth @Jarema

@Jarema
Copy link
Member

Jarema commented Jul 9, 2024

I think the PR is fine.
@bruth any objections, considering you asked for clarifications?

@TheGoderGuy
Copy link

Hi @Jarema @bruth, anything we can do to advance the merge ? We'd really like to implement nack but need this feature :D

Copy link
Member

@bruth bruth left a comment

Choose a reason for hiding this comment

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

Sorry for the delay! LGTM

Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM!

@Jarema Jarema merged commit d0f520d into nats-io:main Jul 26, 2024
3 checks passed
@jabbrwcky
Copy link

Hey, is there a timeline when I can expect this change to show up in an actual release?

AFAICT the latest (0.15.1) release is one week old and I am kind of surprised that it does not contain changes that have been merged to main for a month prior to the release

@jabbrwcky
Copy link

Hi @Jarema,
I saw that there is a new release tag v0.15.2, which still does not include changes since Jul 02.

Just like v0.15.1, github claims that the tagged commit does not belong to any branch of nats-io/nack and the chain of parents of the commits is 7b8b67f (v0.15.2) -> 31a8e48 (v0.15.1) -> 2dd3825 (v0.15.0) with 2dd3825 being the last commit known to github in the repo.

In any case the releases leave out this change and a number of dependabot updates (basically any change on main since Jul 02), so I am curious as to the rationale of these releases.

Either the tags are on an accidentially unpublished branch or this may be intentional. If it is the latter could you share some insight on when th expect the changes to apeear in an release?

@Jarema
Copy link
Member

Jarema commented Sep 10, 2024

Hey. Those are automated patch releases that just bump dependencies.
I will work on release tomorrow.

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.

5 participants