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 metadata-retrieval kind #3

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add metadata-retrieval kind #3

wants to merge 5 commits into from

Conversation

kyrcha
Copy link

@kyrcha kyrcha commented Oct 29, 2019

Added support for regression testing the metadata-retrieval command. Mainly a replication of the gitcollector command.

Signed-off-by: Kyriakos Chatzidimitriou [email protected]

Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
@kyrcha kyrcha added the enhancement New feature or request label Oct 29, 2019
@kyrcha kyrcha requested review from jfontan and lwsanty October 29, 2019 14:29
@kyrcha kyrcha self-assigned this Oct 29, 2019
times = 1
}

// TODO(kyrcha): add a regression.yml file to metadata-retrieval
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 postpone merging this PR until regression.yml appears in metadata-retrieval
it is a must

Copy link
Author

Choose a reason for hiding this comment

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

Created a PR

Name: "metadata-retrieval",
GitURL: "https://github.com/src-d/metadata-retrieval",
ProjectPath: "github.com/src-d/metadata-retrieval",
BinaryName: "cmd",
Copy link
Contributor

Choose a reason for hiding this comment

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

hm, why binary name is cmd?

Copy link
Author

Choose a reason for hiding this comment

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

I've opened an issue for that

Copy link
Contributor

Choose a reason for hiding this comment

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

hm, this might be a problem taking into account your PR, we should wait for this fix before creating the regression branch.
don't you want to discuss this part with applications team and fix it yourself? this will reduce the waiting time for both of this and regression.yml PRs

Copy link
Author

Choose a reason for hiding this comment

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

Yeap on it

test_test.go Outdated
Comment on lines 44 to 45
// No token, no access to teh v4 API
config.GitHubToken = os.Getenv("REG_TOKEN")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused about this part:
It probably does parse of the token with right permissions but you replace it with the token from env, that's not set in the CI or elsewhere

I have certainly missed something, could you please clarify it?

Copy link
Author

Choose a reason for hiding this comment

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

I am doing this assignment because according to the README.md file you have to export REG_TOKEN=your-github-token for this project...so I am doing the assignment here in order to use it in metadataretrieval.go

command := NewCommand(metadataRetrieval.Path, orgs)
	start := time.Now()
	if err := command.Run(map[string]string{
		"LOG_LEVEL":     "debug",
		"GITHUB_TOKENS": t.config.GitHubToken,
	}); err != nil {
		t.log.With(log.Fields{
			"orgs":               orgs,
			"metadata-retrieval": metadataRetrieval.Path,
		}).Errorf(err, "Could not execute metadata-retrieval")
		return nil, err
	}

But you are right...it is not set in the .travis.yml, so I added it:

env:
  - GO111MODULE=on GOPROXY=https://proxy.golang.org
  - REG_TOKEN=$GITHUB_TOKEN

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I see it comes from regression-core's config...


aren't envs should be in one line? because afaik each envs line starts additional job
https://docs.travis-ci.com/user/environment-variables/#defining-multiple-variables-per-item

or am I missing something?

Copy link
Author

Choose a reason for hiding this comment

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

Nope I have already changed it...tests are passing

func TestMetadataRetrieval(t *testing.T) {
config := regression.NewConfig()
config.BinaryCache = "binaries"
config.Versions = []string{"remote:master", "v0.1.0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

afaik {"remote:regression", "remote:master"} order is required

reminder: when regression.yml will be added both branches should contain it, so you would possibly need to create
regression branch

Copy link
Author

Choose a reason for hiding this comment

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

For this I will switch over the conversation to slack.

Copy link
Author

Choose a reason for hiding this comment

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

I will change the order once everything is created.

@jfontan jfontan requested a review from mcarmonaa October 30, 2019 18:07
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
@kyrcha
Copy link
Author

kyrcha commented Oct 31, 2019

Will need to wait for token upgrade for the tests to pass: https://github.com/src-d/infrastructure/issues/1253

Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
@kyrcha
Copy link
Author

kyrcha commented Oct 31, 2019

Token upgraded. Tests are passing. For this PR to be merged still waiting for:

  • Binary name change in [metadata-retrieval]
  • Accepted PR with name regression in [metadata-retrieval]
  • Accepted reviews

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants