Skip to content

Update Docs to point to a simple and more clear example #632

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

Open
wants to merge 195 commits into
base: master
Choose a base branch
from

Conversation

Angr1st
Copy link
Contributor

@Angr1st Angr1st commented May 5, 2020

I personally found the linked build task pretty hard to follow because some of the folder structure was replaced during build time/by some script. I updated my repo to contain a very simple and straightforward example of a build task using the powershell3 task executer. This could be helpful for people with a similar goal that I had. Which was to just package another executable on windows to be run as a build task.

TingluoHuang and others added 30 commits August 29, 2017 10:39
…ent-version

Add TFS 2017 Update 3 agent version.
Deep Dive on Building Custom Build or Deploy Tasks
* Add task.json schema file

* Allow any "connectedService:" type of input

* Added the "id" field
Properly escape property value to handle non string types
* Error on multiline secret

* v2.3.0
AArnott and others added 9 commits January 28, 2020 13:59
Co-authored-by: Tommy Petty <jopetty@microsoft.comwq>
Adding execution options on the pre-job stage.
Shameless self plug -> Microsofts good examples are not clear enough because some parts will be substituted at build time.
added missing s
@Angr1st
Copy link
Contributor Author

Angr1st commented May 5, 2020

Build Erros look like something ran into a timeout or if some tests blocked each other while interacting with the file system. DISCLAIMER: I'm not a js expert. I could be totally off but I haven't touched anything but the one README.md so I'm pretty sure your tests are potentially a little flaky.

@stephenmichaelf stephenmichaelf requested a review from damccorm May 7, 2020 14:08
Copy link

@damccorm damccorm left a comment

Choose a reason for hiding this comment

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

Hey @Angr1st - per internal policy, we try to not link to external examples when possible. I'd prefer linking to an existing example in the tasks repo.

If the MSBuild task is confusing, I'd be open to linking to a simpler task though - maybe useNode?

@Angr1st
Copy link
Contributor Author

Angr1st commented May 7, 2020 via email

@damccorm
Copy link

damccorm commented May 7, 2020

The useNode BuildTask is not a BuildTask that uses the VstsTaskSdk

Whoops, lost context on what part of the doc we were in - you're right. Maybe we can use the cmdLine task

Perhaps you could just fork my repo?

I think I'd rather avoid adding another repo to accomplish this since we do have examples that exercise this. It might be helpful to understand what you found confusing about these examples as well.

@Angr1st
Copy link
Contributor Author

Angr1st commented May 7, 2020 via email

@damccorm
Copy link

damccorm commented May 7, 2020

That's fair, however, these docs are specifically mean to demonstrate consuming the sdk, not setting up the larger task. That's why we link to the ps1 files, not the task folder. We have other docs that specifically talk about the setup and address that.

I think if we're trying to solve that problem, I'd prefer linking to the tutorial over adding another repo to maintain

@Angr1st
Copy link
Contributor Author

Angr1st commented May 7, 2020

You are right about that. I guess I thought you were just presenting examples of how a build task should look like in the end.

The problem I have with just linking to the tutorial would be that this also does not showcase how to use the VstsTaskSdk.

@Angr1st
Copy link
Contributor Author

Angr1st commented May 7, 2020

Instead of linking to another repo I just created an example folder structure inside of the Docs folder. The content of the task.json and task.ps1 should be changed to some microsofty default like Fabrikam or something or what do you think?

@damccorm
Copy link

I'm ok with this approach, but my main issue here is that we don't ever recommend checking powershell modules or binaries into a repo. The pattern we use (and what we'd recommend) is having a pipeline that builds the task, adding in executables as necessary (and that ultimately can publish the task). So I'd be worried about people just copying the repo.

The reasoning behind that is that in any project that has multiple users, checking in binaries introduces a security risk - its extremely challenging to verify the contents of binaries are actually what they should be.

In general, I'd be more in favor of simply making the docs about structure clearer over trying to provide an in-repo example (or even pointing to a stripped down example)

@jessehouwing
Copy link
Contributor

jessehouwing commented May 11, 2020 via email

@damccorm
Copy link

In that case, why isn't there a pre-made script that stores the pipeline
sdk in the right way.

It doesn't exist because nobody's taken the time to do that :) Definitely not opposed to including something like that in the docs. I don't currently have the bandwidth to do that, but I'm happy to review a pr and/or feel free to open up an issue.

For GitHub Actions we also need to commit the SDK directly to the task repo
for it to load

Can't totally speak to that since its a different product (admittedly with some shared stuff), but I believe this is actually not true anymore. They updated their publishing model to use GitHub releases (in large part because of this problem). For example, no node_modules are checked into this repo - https://github.com/actions/stale

@Angr1st
Copy link
Contributor Author

Angr1st commented May 19, 2020

Is there a module manager for powershell? Could you use nuget/paket to ensure reproducable builds (e.g. lock the sdk module version) using a package.lock file? That would be the best way to get the modules at build time I think.

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.