Skip to content

*: clean up samples #4035

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

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Conversation

estroz
Copy link
Member

@estroz estroz commented Oct 14, 2020

Description of the change:

  • Remove old memcached_controller.go.tmpl
  • Add gitignore for trash files
  • Clean up after go make runs

Motivation for the change: fixes CI and remove unnecessary files

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment on lines +113 to +114

*\.DS_Store
Copy link
Member

Choose a reason for hiding this comment

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

+1

@@ -43,7 +41,7 @@ func main() {
os.Exit(1)
}

samplesPath := filepath.Join(currentPath, testdata)
samplesPath := filepath.Join(currentPath, "testdata")
Copy link
Member

Choose a reason for hiding this comment

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

I caught this error on the ansible generator, missed it here. Good catch.

Comment on lines +97 to +99

// Clean up built binaries, if any.
pkg.CheckError("cleaning up", os.RemoveAll(filepath.Join(mh.ctx.Dir, "bin")))
Copy link
Member

Choose a reason for hiding this comment

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

+1 to removing binaries.

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should add this to the gitignore. @camilamacedo86 is doing that in her PR, so no change needed, just FYI.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we will need anyway. To solve the problem in the sanity I added git diff --exit-code -- . ':!testdata/go/memcached-operator/bin/*' since the bin will be gen again in the CI process.

However, the sanity sh will be also be removed in the #4023

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
@estroz estroz force-pushed the chore/cleanup-samples branch from 9dfdf2f to 03271be Compare October 14, 2020 18:57
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

We should also totally add a sanity check for lines that end with whitespace.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2020
Comment on lines -47 to -53

// NewSampleContextWithTestContext returns a SampleContext containing the kubebuilder TestContext informed
// It is useful to allow the samples code be re-used in the e2e tests.
func NewSampleContextWithTestContext(tc *testutils.TestContext) (s SampleContext, err error) {
s.TestContext = *tc
return s, err
}
Copy link
Contributor

@camilamacedo86 camilamacedo86 Oct 14, 2020

Choose a reason for hiding this comment

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

it is usefiul for the e2e test.
I am ok to remove that here but it will be added in the next follow-ups again.

@@ -68,7 +68,7 @@ operator-sdk init --domain example.com --repo github.com/example-inc/memcached-o

## Check if your project is multi-group

Before we start to create the APIs, check if your project has more than one group such as : `foo.example.com/v1` and `crew.example.com/v1`. If you intend to still working on with multiple groups in your project, then add the line
Copy link
Member

Choose a reason for hiding this comment

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

Sidenote, maybe we ought to add these style checks in our linter to make reviews easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's track this need.

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@estroz estroz merged commit c3e2231 into operator-framework:master Oct 14, 2020
@estroz estroz deleted the chore/cleanup-samples branch October 14, 2020 19:43
estroz pushed a commit to estroz/operator-sdk that referenced this pull request Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants