projectkudu / slingshot

This project implements the Deploy To Azure button
http://azure.microsoft.com/blog/2014/11/13/deploy-to-azure-button-for-azure-websites-2/
51 stars 20 forks source link

If branch contains '/' deploy fails #75

Open giuliov opened 7 years ago

giuliov commented 7 years ago

In our project on GitHub we use GitFlow convention supported by many tools, so we have feature/x, release/2.2-rc.1, hotfix/y branch names.

Slingshot fails from such branches, while succeeds for a branch pointing to the same commit but without the forward slash / character.

davidebbo commented 7 years ago

Yeah, seems it's busted in that case.

davidebbo commented 7 years ago

Fix is deployed. Please verify.

giuliov commented 7 years ago

Do you guys ever take vacation 😄?

Thanks it fixed but I noticed another issue in the same scenario: custom parameters are not recognized. See screenshots image image

davidebbo commented 7 years ago

Actually, I found that my change broke some scenarios, so I reverted it for now. Specifically, when including the readme.md in the URL, with my change it starts viewing the branch as master/README.md. e.g.

https://deploy.azure.com/?repository=https://github.com/davidebbo-test/TrivialAppAndWebJob/blob/master/README.md

Worse yet, I'm finding that the GitHub URLs make it very hard to determine the branch. e.g.

https://github.com/davidebbo-test/TrivialAppAndWebJob/tree/foo/bar/App_Data/jobs/triggered/SomeWebJob

Here the branch is foo/bar, and the path within the repo is App_Data/jobs/triggered/SomeWebJob. But there is no way to tell by just looking at the url. Since we support having the azuredeploy.json ins a subfolder, this a problem.

So to summarize, the path can contain 3 things:

  1. optionally the branch. e.g. could be tree/master, or nothing at all if relying on default branch
  2. optionally the folder (i.e. not there if we're at the root)
  3. optionally the .md file

And we somehow need to figure it all out.

In light of this, we need to step back and think about how this can be achieved.

giuliov commented 7 years ago

If I were you I would offer the option to explicitly state the branch in the query string, so https://deploy.azure.com/?repository=https%3A%2F%2Fgithub.com%2Fgiuliov%2Ftfsaggregator&branch=%2Ftree%2Ffeature%2Fwebhooks. In absence of the branch parameter in query string, you let the current 'guessing' algorithm go.

davidebbo commented 7 years ago

It's not that simple, based on how things flow. e.g. look what happens when you go to https://github.com/giuliov/tfsaggregator/blob/feature/webhooks/readme.md and click the button. The button just goes to https://azuredeploy.net/. That then goes into the redirector site, which gets the repo info from the referrer (see code). But at that point we're already dealing with ambiguous info.

deploy.azure.com could certainly accept an alternate way to get the branch on the query string, but then authoring the readme will become tricky, as you'd need to change it for each branch to hard code the branch, and that's pretty nasty.

I think it should be possible to have a heuristic which works in the wide majority of scenarios, with more complex logic. e.g.

I think in practice it would work well, unless someone goes out of their way to make it ambiguous, and we could live with that.

Please note that we have limited resources to work on slingshot, so if you want to make something happen, a PR would help :)

giuliov commented 7 years ago

I will try as it seems reasonably simple to debug locally, but do not expect anything soon: I have my own GitHub project to support in spare time. Can you assign the issue to me?

giuliov commented 7 years ago

There are instructions at hand on building this? I get a This project is incompatible with the current version of Visual Studio on VS2017RC.

davidebbo commented 7 years ago

Can you use VS2015 instead? VS2017 is still rough, and might be broken in all kind of ways.

giuliov commented 7 years ago

That works, now I have to setup the AAD environment... you'll hear from me when I have news. Sleep well.