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

If branch contains '/' deploy fails #75

Open
giuliov opened this issue Nov 25, 2016 · 10 comments
Open

If branch contains '/' deploy fails #75

giuliov opened this issue Nov 25, 2016 · 10 comments
Assignees

Comments

@giuliov
Copy link

giuliov commented Nov 25, 2016

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
Copy link
Member

Yeah, seems it's busted in that case.

@davidebbo
Copy link
Member

Fix is deployed. Please verify.

@giuliov
Copy link
Author

giuliov commented Nov 26, 2016

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
Copy link
Member

davidebbo commented Nov 26, 2016

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.

@davidebbo davidebbo reopened this Nov 26, 2016
davidebbo added a commit that referenced this issue Nov 26, 2016
@giuliov
Copy link
Author

giuliov commented Nov 26, 2016

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
Copy link
Member

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.

  • Get the list of branches from GitHub API. e.g. see https://api.github.com/repos/giuliov/tfsaggregator/branches
  • Find the longest (i.e. most tokens) branch name in the list that matches the beginning of the path, and use that
  • Whatever is left is the path within the repo, with optionally the .md file name at the end

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
Copy link
Author

giuliov commented Nov 28, 2016

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
Copy link
Author

giuliov commented Nov 28, 2016

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
Copy link
Member

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

@giuliov
Copy link
Author

giuliov commented Nov 28, 2016

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

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

No branches or pull requests

2 participants