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

Setup priv package #1

Merged
merged 7 commits into from
Dec 3, 2024
Merged

Setup priv package #1

merged 7 commits into from
Dec 3, 2024

Conversation

Argonus
Copy link
Collaborator

@Argonus Argonus commented Dec 2, 2024

Summary of changes

I'll review the commits, so I mostly want to understand the "why" rather than the "what"

Checklist

  • New functions have typespecs, changed functions were updated
  • Same for documentation, including moduledocs
  • Tests were added or updated to cover changes
  • Commits were squashed into a single coherent commit
  • Notes added to CHANGELOG file which describe changes at a high-level

mix.exs Outdated Show resolved Hide resolved
@Argonus Argonus force-pushed the publish-forked-package branch from b4f45b7 to 103931e Compare December 2, 2024 15:32
Add resource version parameter for kubernates strategy.
@Argonus Argonus marked this pull request as ready for review December 3, 2024 07:19
@Argonus Argonus force-pushed the publish-forked-package branch from a2a3127 to 76dba55 Compare December 3, 2024 07:25
@Argonus Argonus requested a review from krukonshedul December 3, 2024 07:25
Copy link

@emancu emancu left a comment

Choose a reason for hiding this comment

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

Change seems ok for me. I dont fully understand why you use resourceVersion, what is the reasoning behind that change.

How does this change will reduce the API calls to kubernetes 🤔 ?

mix.exs Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
.github/workflows/ci.yaml Show resolved Hide resolved
@@ -1,7 +1,7 @@
defmodule Cluster.Mixfile do
use Mix.Project

@version "3.4.1"
@version "4.0.0"
Copy link

Choose a reason for hiding this comment

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

🤔 Is this really a major change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@emancu it's just our fork, not origin.

Copy link

Choose a reason for hiding this comment

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

But why not doing changes taht can be contributed back to the original project?

@Argonus Argonus merged commit 9475f03 into main Dec 3, 2024
5 of 6 checks passed
@Argonus Argonus deleted the publish-forked-package branch December 3, 2024 10:55
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