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

Fork helm-operator adjustments for CI and documentation #4

Merged
merged 3 commits into from
May 18, 2021
Merged

Conversation

SimonBaeumer
Copy link
Member

@SimonBaeumer SimonBaeumer commented May 18, 2021

CI migration of the helm-operator fork.
Did not deleted the original deploy actions.

  • Disable some docker image push
  • Execute CI on all pushes

@SimonBaeumer SimonBaeumer changed the title Add CI Adjust CI to fork, disable image push May 18, 2021
@SimonBaeumer SimonBaeumer changed the title Adjust CI to fork, disable image push Fork helm-operator adjustments for CI and documentation May 18, 2021
README.md Outdated

Experimental refactoring of the operator-framework's helm operator

### Why a fork?
Copy link
Member Author

Choose a reason for hiding this comment

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

@gaurav-nelson Do you have any feedback for me here?
I would like to say that this is a fork which diverges in its use-case from the original operator implementation.

Copy link

@msugakov msugakov left a comment

Choose a reason for hiding this comment

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

I guess it would be good to have some convention how to label commits for easier upstreaming our changes. For example, this commit must not be picked for upstreaming and can be marked somehow like [not for upstream] Fork helm-operator adjustments for CI and documentation.
What do you think?

- '**'
pull_request:
branches: [ main ]
- push
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious as to why you changed this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because the CI was not triggered in the default setting.

README.md Outdated
As the helm-operator is an experimental refactoring and not actively maintained we started a fork to
further support [hybrid operators](https://github.com/operator-framework/operator-sdk/issues/670) based on Helm.

This fork should used as a library and not is recommended to watch CustomResources by configured `watches`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this sentence, or how "watching CustomResources by configured watches" relates to whether or not it's being used as a library

Copy link
Member Author

Choose a reason for hiding this comment

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

Rephrased the whole paragraph.

README.md Outdated
Add this lib as a replace directive to your `go.mod`:

```
replace(
Copy link
Contributor

Choose a reason for hiding this comment

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

super nit: it's common to just provide a CLI command, e.g., go mod edit -replace=github.com/joelanford/helm-operator=github.com/stackrox/helm-operator@main (also, the line as it's written is invalid, as you need to specify a revision for the replacement module)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@SimonBaeumer
Copy link
Member Author

I guess it would be good to have some convention how to label commits for easier upstreaming our changes. For example, this commit must not be picked for upstreaming and can be marked somehow like [not for upstream] Fork helm-operator adjustments for CI and documentation.
What do you think?

👍
Added labels for it, by default upstream-triage is added to indicate that it should be decided if this should be an upstream fix too.

@SimonBaeumer SimonBaeumer merged commit a116e55 into main May 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants