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

Bitbucket enterprise support #246

Merged
merged 10 commits into from May 17, 2019

Conversation

adam-sajdak
Copy link
Contributor

Add support for Bitbucket Enterprise Edition.

  • Only one package is created for supporting different flavors of Bitbucket
  • There is need for manual host configuration in csproj. Similar as in TFS package
  • Supports url for 3 formats: Cloud, Enterprise Edition prior to 4.7, Enterprise Edition 4.7 and above

@dnfclas
Copy link

dnfclas commented Feb 21, 2019

CLA assistant check
All CLA requirements met.

@ctaggart
Copy link
Contributor

ctaggart commented Mar 8, 2019

@adam-sajdak Are you able to sign the CLA?

@walliski
Copy link

@ctaggart Is there something that can be done in case @adam-sajdak is unable to sign it?

Me and a bunch of other people seem to eagerly wait for this feature to be released.

@tmat
Copy link
Member

tmat commented Mar 15, 2019

@walliski I don't think there is, other than reimplement it in a new PR.

@adam-sajdak
Copy link
Contributor Author

Hello,
it takes long time, but I hope that I will be able to sign the CLA soon. I will keep you updated.

@tmat
Copy link
Member

tmat commented Apr 23, 2019

@adam-sajdak Friendly reminder.

@ctaggart
Copy link
Contributor

@adam-sajdak, it has been a month since we last heard from you and two months since this pull request was opened. Is legal still holding up things?

@tmat
Copy link
Member

tmat commented Apr 29, 2019

@adam-sajdak Can you estimate when/if you will be able to sign the CLA?

…tbucket_Enterprise_Support

# Conflicts:
#	src/SourceLink.Bitbucket.Git/build/Microsoft.SourceLink.Bitbucket.Git.props
#	src/SourceLink.Bitbucket.Git/buildCrossTargeting/Microsoft.SourceLink.Bitbucket.Git.props
@adam-sajdak
Copy link
Contributor Author

Hi sorry for big delay I finally signed CLA.

@tmat
Copy link
Member

tmat commented Apr 30, 2019

Cool, I'll take a look.

README.md Outdated

```xml
<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.Bitbucket.Git" Version="1.0.0-beta2-18618-05" PrivateAssets="All"/>
<SourceLinkBitbucketGitHost Include="bitbucket.org" EnterpriseEdition="true" Version="4.1" />
Copy link
Member

Choose a reason for hiding this comment

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

[](start = 2, length = 93)

This should be default. Customer who uses repository hosted on bitbucket.org shouldn't need to specify anything in their project file.

Copy link
Member

Choose a reason for hiding this comment

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

When using a server hosted in different domain we should default to enterprise edition and the latest version with the option to specify an older version.


In reply to: 280532565 [](ancestors = 280532565)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.
Now for bitbucket.org cloud hosting there is no need to provide SourceLinkBitbucketGitHost configuration in csproj.
When the domain is different than bitbucket.org it defaults to enterprise edition and the latest version.

protected override bool SupportsImplicitHost => false;

protected override string BuildSourceLinkUrl(Uri contentUri, Uri gitUri, string relativeUrl, string revisionId,
ITaskItem hostItem)
Copy link
Member

Choose a reason for hiding this comment

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

ITaskItem hostItem [](start = 12, length = 18)

Style: keep the parameter on the previous line. We either indent all parameters or none.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

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

🕐

@kvskranthikumar
Copy link

Any idea, how long will it take to get released.

@adam-sajdak
Copy link
Contributor Author

Any idea, how long will it take to get released.

I am in the middle of changes code review comments and I will update pull request tomorrow.

Adding SourceLinkBitbucketGitHost is not necessary for cloud scenario
For self-hosting scenario adding SourceLinkBitbucketGitHost  with proper host is required
EnterpriseEdition and Version settings are optional with default values.
@adam-sajdak
Copy link
Contributor Author

🕐

🕐

@tmat
Please take a look at my changes for you code review comments.

README.md Outdated

```xml
<ItemGroup>
<PackageReference Include="Microsoft.SourceLink.Bitbucket.Git" Version="1.0.0-beta2-18618-05" PrivateAssets="All"/>
</ItemGroup>
```

For self-hosted Bitbucket projects reference [Microsoft.SourceLink.Bitbucket.Git](https://www.nuget.org/packages/Microsoft.SourceLink.Bitbucket.Git) package and add Bitbucket host configuration.
Additionally there are two optional attributes with default values:
EnterpriseEdition="true" - flag whether it is Enterprise Edtition or Cloud Edition
Copy link
Member

Choose a reason for hiding this comment

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

Edtition [](start = 57, length = 8)

typo: Edtition

Copy link
Member

Choose a reason for hiding this comment

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

I don't think markdown will format these as you intended - on separate lines.


In reply to: 283563555 [](ancestors = 283563555)

Copy link
Member

Choose a reason for hiding this comment

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

Let's include the EnterpriseEdition and Version parameters to the example below.


In reply to: 283563725 [](ancestors = 283563725,283563555)

private const string VersionMetadataName = "Version";
private const string VersionWithNewUrlFormat = "4.7";

protected override bool SupportsImplicitHost => false;
Copy link
Member

Choose a reason for hiding this comment

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

SupportsImplicitHost [](start = 32, length = 20)

If you keep this set to true (the default) it won't be necessary to specify the host, unless the user needs to specify the version and edition. I think that would be better. Specifying the domain when the defaults are used is unnecessary.

@@ -13,7 +13,7 @@ public BitBucketGitTests()
}

[ConditionalFact(typeof(DotNetSdkAvailable))]
public void FullValidation_Https()
public void FullValidation_CloudHttps()
Copy link
Member

Choose a reason for hiding this comment

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

FullValidation_CloudHttps [](start = 20, length = 25)

This is odd - we say that the default is EnterpriseEdition="true", but the bitbucket.org host specified in src\SourceLink.Bitbucket.Git\build\Microsoft.SourceLink.Bitbucket.Git.props should be using the Cloud Edition URl format, right?

Shouldn't the SourceLinkBitbucketGitHost for bitbucket.org have EnterpriseEdition="false"?

<SourceLinkBitbucketGitHost Include="bitbucket.org" EnterpriseEdition="false" />

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see, you're special casing bitbucket.org in the code:

if (!BitbucketCloudHostingUrl.Equals(gitUri.GetHost(), StringComparison.OrdinalIgnoreCase))

We can remove that special case and specify the property in the props file directly.


In reply to: 283574378 [](ancestors = 283574378)

protected override string BuildSourceLinkUrl(Uri contentUri, Uri gitUri, string relativeUrl, string revisionId, ITaskItem hostItem)
=> UriUtilities.Combine(UriUtilities.Combine(contentUri.ToString(), relativeUrl), "raw/" + revisionId + "/*");
{
var isEnterpriseEditionFlagAvailable =
Copy link
Member

Choose a reason for hiding this comment

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

isEnterpriseEditionFlagAvailable [](start = 16, length = 32)

This can be simplified as

return boo.TryParse(..., out var isEnterpriseEditionValue) && isEnterpriseEditionValue ?
            BuildSourceLinkUrlForEnterpriseEdition(contentUri, relativeUrl, revisionId, hostItem) : 
            BuildSourceLinkUrlForCloudEdition(contentUri, relativeUrl, revisionId); 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tmat I modified pull request according to your suggestions.

  • Now the SourceLinkBitbucketGitHost for bitbucket.org in props have EnterpriseEdition="false"
  • Removed SupportsImplicitHost => false;
  • I had to check if hostItem is null. It is null if SourceLinkBitbucketGitHost does not match actual git url host.

Bottom line, now it will be not necessary to put SourceLinkBitbucketGitHost in csproj(unless you want to set specifiv Version attribute) and it will work for both cloud and enterprise scenarios.

Add EnterpriseEdition="false" in defaulp props file so Bitbucket.org is recognized as cloud version
By default EnterpriseEdition is true so any other bitbucket repository is recognized as enterprise version by default
@tmat
Copy link
Member

tmat commented May 16, 2019

Looks great. Thanks for making all the changes.

I'm wondering if we could simplify a bit further. It just occurred to me that since the version is only relevant for Enterprise Edition, we could just have a single attribute EnterpriseEditionVersion. If it is not specified, it's a Cloud Edition. Otherwise it is an Enterprise Edition with the specified version. I'll leave it up to you if you want to make the change. I'm ok merging the PR as is now.

@adam-sajdak
Copy link
Contributor Author

Looks great. Thanks for making all the changes.

I'm wondering if we could simplify a bit further. It just occurred to me that since the version is only relevant for Enterprise Edition, we could just have a single attribute EnterpriseEditionVersion. If it is not specified, it's a Cloud Edition. Otherwise it is an Enterprise Edition with the specified version. I'll leave it up to you if you want to make the change. I'm ok merging the PR as is now.

Great, thank you. For me the current state is fine and I think we can merge it. Could you do it @tmat ?

@tmat tmat merged commit 8144155 into dotnet:master May 17, 2019
@tmat
Copy link
Member

tmat commented May 17, 2019

@adam-sajdak Thanks!

@walliski
Copy link

@tmat Would it be possible to get a new release with these changes?

@kvskranthikumar
Copy link

+1
Waiting for the release.

@tmat
Copy link
Member

tmat commented May 20, 2019

@kvskranthikumar
Copy link

I have verified the fix, it is working perfectly :)

<ItemGroup>
		<PackageReference Include="Microsoft.SourceLink.Bitbucket.Git" Version="1.0.0-beta2-19270-01" PrivateAssets="All"/>
		<SourceLinkBitbucketGitHost Include="company.com" EnterpriseEdition="true" Version="5.12.1"/>
	</ItemGroup>

When will this be released to nuget.org, currentlly the version available on nuget.org is 1.0.0-beta2-18618-05

@tmat
Copy link
Member

tmat commented May 21, 2019

I'll publish is asap. Just wanted to double check it works. Glad to hear it does.

@tmat
Copy link
Member

tmat commented May 23, 2019

A bit confused about the names "Enterprise Edition" vs "Cloud Edition".

Based on https://confluence.atlassian.com/confeval/development-tools-evaluator-resources/bitbucket/bitbucket-what-is-bitbucket my understanding is that Cloud Edition is the one running on bitbucket.org and never self-hosted. Enterprise Edition would be either Bitbucket Server or Bitbucket Data Center. Is that correct?

@tmat
Copy link
Member

tmat commented May 24, 2019

BTW, the packages are now available on nuget.org: https://www.nuget.org/packages?q=Microsoft.SourceLink

@adam-sajdak
Copy link
Contributor Author

A bit confused about the names "Enterprise Edition" vs "Cloud Edition".

Based on https://confluence.atlassian.com/confeval/development-tools-evaluator-resources/bitbucket/bitbucket-what-is-bitbucket my understanding is that Cloud Edition is the one running on bitbucket.org and never self-hosted. Enterprise Edition would be either Bitbucket Server or Bitbucket Data Center. Is that correct?

"Cloud Edition" is Bitbucket Cloud, so the one hosted on Atlassian's bitbucket.org.
"Enterprise Edition" is Bitbucket Server or Bitbucket Data Center, basically the Bitbucket that is hosted on-premise.
@tmat do you think it is confusing and we should change the wording everywhere so insted "Enterprise Edition" we use "On-premise Edition"?

@tmat
Copy link
Member

tmat commented May 24, 2019

That's my understanding now as well. I have already updated the readme.

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.

None yet

6 participants