-
Notifications
You must be signed in to change notification settings - Fork 2
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
times = 1 | ||
} | ||
|
||
// TODO(kyrcha): add a regression.yml file to metadata-retrieval |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
// No token, no access to teh v4 API | ||
config.GitHubToken = os.Getenv("REG_TOKEN") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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"} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
Signed-off-by: Kyriakos Chatzidimitriou <[email protected]>
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]>
Token upgraded. Tests are passing. For this PR to be merged still waiting for:
|
Added support for regression testing the metadata-retrieval command. Mainly a replication of the gitcollector command.
Signed-off-by: Kyriakos Chatzidimitriou [email protected]