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

ccl: refactor tenant status server test to statusccl package #72556

Merged
merged 1 commit into from
Nov 11, 2021

Conversation

Azhng
Copy link
Contributor

@Azhng Azhng commented Nov 9, 2021

Previously, tenant status server tests live within serverccl package.
However, this caused all nightly test failures to automaticallly
notify server team through GitHub code owner file.
This commit moves all tenant status server into statusccl package
and update statusccl package code owner to SQL O11y.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@Azhng Azhng force-pushed the statusccl branch 2 times, most recently from bec86d9 to 80efd50 Compare November 9, 2021 15:57
@Azhng Azhng requested review from a team and knz November 9, 2021 15:57
@Azhng Azhng marked this pull request as ready for review November 9, 2021 15:57
Copy link
Contributor

@lindseyjin lindseyjin left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/ccl/serverccl/statusccl/main_test.go, line 15 at r1 (raw file):

	"testing"

	_ "github.com/cockroachdb/cockroach/pkg/ccl"

Btw, is there supposed to be an underscore here? What does that mean?

Copy link
Contributor Author

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/ccl/serverccl/statusccl/main_test.go, line 15 at r1 (raw file):

is there supposed to be an underscore here

Yes.

What does that mean?

When importing a package, all the symbols within the package will be available under the package name, e.g.

import "github.com/cockroachdb/cockroach/pkg/security"

//...  later in the same file

security.ExportedSymbols // <- exported symbols are available under the `security` package name

However, in golang, there is a special function called init, which is defined as :

package some_package_name

func init() {
   // does stuff 
}

This function is not exported since it's lowercase. However, if this package is being imported by some other package, then the init function will be executed upon startup. This is done in ccl package to inject ccl functionalities into the CRDB

Copy link
Contributor

@lindseyjin lindseyjin left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/ccl/serverccl/statusccl/main_test.go, line 15 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

is there supposed to be an underscore here

Yes.

What does that mean?

When importing a package, all the symbols within the package will be available under the package name, e.g.

import "github.com/cockroachdb/cockroach/pkg/security"

//...  later in the same file

security.ExportedSymbols // <- exported symbols are available under the `security` package name

However, in golang, there is a special function called init, which is defined as :

package some_package_name

func init() {
   // does stuff 
}

This function is not exported since it's lowercase. However, if this package is being imported by some other package, then the init function will be executed upon startup. This is done in ccl package to inject ccl functionalities into the CRDB

Ah, I see, so we're importing the package just to execute the init function?

Copy link
Contributor Author

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/ccl/serverccl/statusccl/main_test.go, line 15 at r1 (raw file):

Previously, lindseyjin (Lindsey Jin) wrote…

Ah, I see, so we're importing the package just to execute the init function?

for pkg/ccl here yes 👍

Previously, tenant status server tests live within serverccl package.
However, this caused all nightly test failures to automaticallly
notify server team through GitHub code owner file.
This commit moves all tenant status server into statusccl package
and update statusccl package code owner to SQL O11y.

Release note: None
Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

just one small nit and :lgtm:

Reviewed 7 of 8 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @knz)


pkg/ccl/serverccl/statusccl/main_test.go, line 34 at r2 (raw file):

}

//go:generate ../../../util/leaktest/add-leaktest.sh *_test.go

nit: did you forget to remove this comment?

Copy link
Contributor Author

@Azhng Azhng left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz and @maryliag)


pkg/ccl/serverccl/statusccl/main_test.go, line 34 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: did you forget to remove this comment?

I think this comment was originally explicitly left to to ensure leak test are injected into all unit tests.

@Azhng
Copy link
Contributor Author

Azhng commented Nov 11, 2021

TFTR!

bors r=maryliag

@craig
Copy link
Contributor

craig bot commented Nov 11, 2021

Build succeeded:

@craig craig bot merged commit 503f136 into cockroachdb:master Nov 11, 2021
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.

4 participants