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 a flag to skip tests using Cloud Logging API #57

Closed
kyasbal opened this issue Feb 4, 2025 · 6 comments · Fixed by #69
Closed

Add a flag to skip tests using Cloud Logging API #57

kyasbal opened this issue Feb 4, 2025 · 6 comments · Fixed by #69
Assignees
Labels
good first issue Good for newcomers

Comments

@kyasbal
Copy link
Member

kyasbal commented Feb 4, 2025

We need few weeks to get Workload Identity Pool in our testing project and we can't enable backend tests using Cloud Logging on CI until that.

Let's make a flag to skip these tests for running these tests in current CI without using Cloud Logging.

@kyasbal kyasbal added the good first issue Good for newcomers label Feb 4, 2025
@Okabe-Junya
Copy link
Contributor

@kyasbal

Hello, and first of all, thank you for releasing such amazing software!

I'm very interested in this software, and when I tried to hack it, I noticed that running make test (i.e. go test ./...) fails. So, I'd like to work on this issue.

There are several ways to operate on the files under test in Go, but which method would be best (or another)?

  1. Create an internal/testflags package and blank-import it in every package that runs tests (treating the flags as global variables; otherwise, it probably won’t be possible to run it as go test ./... -args <myflag>=true).
  2. Use the built-in short (go test -short) flag.
  3. Use environment variables as flags.

If this issue is only a temporary change until the Workload Identity Pool becomes available, then using environment variables would be the simplest approach.
However, if you plan to continue running make test (or make test-local) in the development environment going forward, creating a dedicated package for test flags would be worthwhile.

Thanks!

@kyasbal
Copy link
Member Author

kyasbal commented Feb 8, 2025

@Okabe-Junya Thank you for being interested in contributing this project 🙇

There are several ways to operate on the files under test in Go, but which method would be best (or another)?
Create an internal/testflags package and blank-import it in every package that runs tests (treating the flags as global variables; otherwise, it probably won’t be possible to run it as go test ./... -args =true).
Use the built-in short (go test -short) flag.
Use environment variables as flags.

If this issue is only a temporary change until the Workload Identity Pool becomes available, then using environment variables would be the simplest approach.
However, if you plan to continue running make test (or make test-local) in the development environment going forward, creating a dedicated package for test flags would be worthwhile.

I prefer the first way. It's because we'll support OSS kubernetes logs or other log storage in future and it needs to call some APIs not on Google Cloud. In the case, I also want to disable calling it in my test because I don't want to prepare every environment.
This would be same for contributor only interested in OSS kubernetes log feature part and wanting to ignore tests strictly related to Google Cloud. It's should be okey eventually tested in CI before merging. We should keep capability to disable them in local environment.

By the way, we have pkg/common/flag package which reads program argument and environment variable only when it the program argument has corresponding environment variable key. Do you think if this package is usable for this purpose? If you think it's needs to be the default flag package in Go, maybe like because of cyclic import or something, you can use the default flag package instead.
Test commands are written in Makefile and I don't want all contributors to customize the Makefile to match their needs. If the flag can be disabled or enabled by each local different development environment, it would be preferable to specify in environment variable instead of program argument because they can set that with direnv or something to customize only on their env. I would like to go the hybrid way of the first option and 2nd option because of this reason if we could.

Again, thank you for being interested in this project 👍

@Okabe-Junya
Copy link
Contributor

Thanks!

First of all, I apologize for my delay in response - I took days off.

By the way, we have pkg/common/flag package which reads program argument and environment variable only when it the program argument has corresponding environment variable key. Do you think if this package is usable for this purpose?

I think we should clearly separate the packages. It would be better to create a package for managing test flags under the internal/ directory. Furthermore, since test flags are managed via variables, they should be kept separate from the pkg/common/flag namespace.

I would like to go the hybrid way of the first option and 2nd option because of this reason if we could.

The second option is the approach that uses short flags, but do you mean a combination of the first and the third (i.e. using an internal package and environment variables)?

In any case, it seems we have reached a consensus on the overall strategy.
(I will likely create the initial PR on Thursday this week)

Thank you :)

@kyasbal
Copy link
Member Author

kyasbal commented Feb 10, 2025

The second option is the approach that uses short flags, but do you mean a combination of the first and the third (i.e. using an internal package and environment variables)?

Yes, you are right. I meant it's combination of 1st and 3rd.

In any case, it seems we have reached a consensus on the overall strategy.

Yes, and we can discuss further details with the code reviews with actual codes. That would be far easier to discuss.

(I will likely create the initial PR on Thursday this week)

Thank you! You can take your time only when you want to, this is an OSS project.

@kyasbal kyasbal assigned kyasbal and Okabe-Junya and unassigned kyasbal Feb 10, 2025
@kyasbal
Copy link
Member Author

kyasbal commented Feb 10, 2025

I'll mark this issue assigned to you but this is just to make it clear that this good-first-issue have a contributor.
You can unassign yourself whenever you don't want to.

@Okabe-Junya
Copy link
Contributor

Hello @kyasbal

I opened #69 - please take a look to move forward this issue :)

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants