Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

Fixed a bug where the repo:has.description() parameter shows repo name instead of description on Bitbucket server repos#46752

Merged
mibali merged 12 commits into
mainfrom
mibali/bbserver-test
Jan 24, 2023
Merged

Fixed a bug where the repo:has.description() parameter shows repo name instead of description on Bitbucket server repos#46752
mibali merged 12 commits into
mainfrom
mibali/bbserver-test

Conversation

@mibali

@mibali mibali commented Jan 21, 2023

Copy link
Copy Markdown
Contributor

fixes #46713

Previously, we set the description to be the repo name (See here). This made the repo:has.description parameter shows repo name instead of the description on Bitbucket server repos.

In this PR, I added a Description type for the Repo struct and then modified the Description in the BitbucketServerSource function to point to the description. This would make the repo:has.description parameter to now show the actual description instead of the repo name.

Test plan

Tested locally.

@mibali mibali requested a review from indradhanush January 21, 2023 01:54
@cla-bot cla-bot Bot added the cla-signed label Jan 21, 2023
@sourcegraph-bot

sourcegraph-bot commented Jan 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codenotify: Notifying subscribers in CODENOTIFY files for diff 88234f3...e7b72d1.

Notify File(s)
@eseliger enterprise/internal/batches/sources/testdata/golden/TestBitbucketServerSource_GetUserFork_already_forked
enterprise/internal/batches/sources/testdata/golden/TestBitbucketServerSource_GetUserFork_new_fork
enterprise/internal/batches/sources/testdata/sources/TestBitbucketServerSource_GetUserFork_already_forked.yaml
enterprise/internal/batches/sources/testdata/sources/TestBitbucketServerSource_GetUserFork_bad_username.yaml
enterprise/internal/batches/sources/testdata/sources/TestBitbucketServerSource_GetUserFork_new_fork.yaml
enterprise/internal/batches/sources/testdata/sources/TestBitbucketServerSource_GetUserFork_not_a_fork.yaml
enterprise/internal/batches/sources/testdata/sources/TestBitbucketServerSource_GetUserFork_not_forked_from_parent.yaml
internal/extsvc/bitbucketserver/client.go
internal/extsvc/bitbucketserver/testdata/golden/CreateFork
internal/extsvc/bitbucketserver/testdata/golden/LabeledRepos-archived
internal/extsvc/bitbucketserver/testdata/golden/ProjectRepos
internal/extsvc/bitbucketserver/testdata/vcr/CreateFork.yaml
internal/extsvc/bitbucketserver/testdata/vcr/LabeledRepos.yaml
internal/extsvc/bitbucketserver/testdata/vcr/ProjectRepos.yaml
@indradhanush internal/repos/bitbucketserver.go
internal/repos/bitbucketserver_test.go
@ryanslade internal/repos/bitbucketserver.go
internal/repos/bitbucketserver_test.go
@sashaostrikov internal/repos/bitbucketserver.go
internal/repos/bitbucketserver_test.go

@mibali mibali changed the title Modification to include BB server description Fixed a bug where the repo:has.description() parameter shows repo name instead of description on Bitbucket server repos Jan 22, 2023
indradhanush
indradhanush suggested changes Jan 23, 2023
View reviewed changes

@indradhanush indradhanush left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great PR @mibali and thank you so much for the fix! ❤️

The changes look good to me however the tests are failing and would need to be updated. For API tests we use real API response data but save it and reuse it for subsequent test runs. In this case since we're adding a new field that we expect from the API, we should update the tests by adding the -update flag to the test command. In this case I can see two relevant tests that need to be updated:

  1. TestClient_LabeledRepos
  2. TestClient_ProjectRepos

To update them you will need access to an API token for our test instance which you can find in 1pass. You may run the tests like:

cd internal/extsvc/bitbucketserver
BITBUCKET_SERVER_TOKEN=<insert-token-here> go test -v -run TestClient_LabeledRepods -update

Repeat this for all the other test that need to be updated.

Additionally I would also recommend to add a description to one of the repos in the instance which get fetched from our tests and see if that is reflecting in the newly generated testdata. This should help with validating in our tests that the new changes work.

Comment thread internal/repos/bitbucketserver.go Outdated
sashaostrikov
sashaostrikov reviewed Jan 23, 2023
View reviewed changes

@sashaostrikov sashaostrikov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the fix!
Agree on what Indra wrote, after fixing the test it is good to merge.

@mibali mibali requested a review from indradhanush January 24, 2023 11:16
indradhanush
indradhanush suggested changes Jan 24, 2023
View reviewed changes
Comment thread internal/repos/testdata/vcr/path-pattern.yaml Outdated
status: 200 OK
code: 200
- '@TCJ96Jx619x10993429x1'
status: 401 Unauthorized

@indradhanush indradhanush Jan 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This doesn't look right to me. These are the files that I updated. Let me take a look.

@indradhanush indradhanush Jan 24, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Okay I fixed the token in the test run and I'm seeing the same timeout error as you. I'll investigate this.

@mibali mibali Jan 24, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Interestingly Buildkite is all green on my end

@indradhanush indradhanush Jan 24, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yep it is, but the generated file is not correct. Take a look at the status: 401 Unauthorized. This was likely me having a typo in the token. I've fixed it with the correct token and I am now seeing the timeout locally.

indradhanush
indradhanush reviewed Jan 24, 2023
View reviewed changes
Comment thread internal/repos/bitbucketserver_test.go Outdated
@mibali mibali merged commit 7833ec0 into main Jan 24, 2023
@mibali mibali deleted the mibali/bbserver-test branch January 24, 2023 15:01
mibali pushed a commit that referenced this pull request Jan 30, 2023
@topebali
Added the bug fix #46752 to the changelog
5b13c99
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@sashaostrikov sashaostrikov sashaostrikov left review comments

@indradhanush indradhanush indradhanush approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

repo:has.description parameter shows repo name instead of description on Bitbucket server repos

6 participants

@mibali @sourcegraph-bot @indradhanush @sashaostrikov @topebali @mrnugget