Skip to content
This repository has been archived by the owner on Feb 24, 2021. It is now read-only.

Join "Default Shared AppVeyor Model" and "Harness AppVeyor Model" to one #225

Open
johlju opened this issue May 21, 2018 · 20 comments
Open
Labels
enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.

Comments

@johlju
Copy link
Contributor

johlju commented May 21, 2018

I suggest we look into joining the both modules so that a TestHarness.psm1 file is not needed when using the "Harness AppVeyor Model". The "Default Shared AppVeyor Model" has more functionality that "Harness AppVeyor Model" resource modules can't use because all test logic is in the

The file that is called from the test framework doesn't seem so complicated.
https://github.com/PowerShell/ComputerManagementDsc/blob/dev/Tests/TestHarness.psm1

The biggest difference (only difference) is that there are a different folder structure in the resource modules that using the "Harness model".

@johlju johlju added enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community. labels May 21, 2018
@johlju
Copy link
Contributor Author

johlju commented May 21, 2018

@PlagueHO is this possible to accomplish?

@johlju
Copy link
Contributor Author

johlju commented May 21, 2018

The "Harness AppVeyor Model" also uses a special variant of the test templates that need to be updated so only one test template is used that works for both folder structures.

https://github.com/PowerShell/ComputerManagementDsc/blob/a028421c8b81b7018dcc6c58346ca735d627358a/Tests/Unit/MSFT_ScheduledTask.Tests.ps1#L12

@PlagueHO
Copy link
Collaborator

@johlju - it may be possible. But there are also some differences in some harnesses (especially in SharePointDsc) - not just in name either. And in the tests themselves.

But I'd suggest an iterative change approach rather than big bang to do this - so that if it can't be implemented or turns out to have negative side-effects then it isn't going to be as painful to back out or work around.

I'd suggest the following just trying the following:

  1. Generalize the test harness function Invoke-TestHarness and move it into DSCResource.Tests into the TestHelper.psm1 module (shouldn't be in AppVeyor as this harness is required to run tests locally as well).
  2. Implement a check in AppVeyor.psm1 that will execute the TestHarness inside the repo if it exists or if not then uses the one built into TestHelper.psm1.
  3. Finally remove the TestHarness from each repo.

But I think as @tysonjhayes can attest, there are a number of changes that had to happen to implement this auto-documentation model of resource.

@PlagueHO
Copy link
Collaborator

The other thing to remember is that SharePointDsc and OfficeOnlineServerDsc
also uses the harness method (any of the auto-documentation resources) so we'd want to make sure anything that is done works for those projects too.

@johlju
Copy link
Contributor Author

johlju commented May 22, 2018

Yes, if we can do this it should be seamless. The auto-documentation feature can't be used in "default" yet, I think? I haven't tested that yet. There are functionality in both that either can't use yet.
I think we should take what best of both and make a single "model".

What is the biggest gain of using the folder structure that is used for the "Harness AppVeyor Model"?

@johlju
Copy link
Contributor Author

johlju commented May 22, 2018

@PlagueHO I saw in another repo that you publish the Wiki manually after release? What steps do you do to accomplish that?

@PlagueHO
Copy link
Collaborator

Hi @johlju - the auto-documentation feature currently requires the folder structure (which requires the harness). Theoretically if we could find a way of getting rid of the folder structure while still allowing the auto-documentation to work then we could probably get rid of the harness and move to a unified model.

This should be possible with the repos I maintain, but it's the SharePointDsc etc that I'm not sure about. I'm not certain if the folder structure and harness is for other reasons.

We could certainly try this out in one of the repos I maintain and see what is required.

The process for publishing the Wiki content is currently manual and requires performing a Git push on the Wiki pages after unzipping the Wiki content artefact that is produced by the CI. I wanted to be able to automate this process (#142), but as usual I've not got round to this one (not enough hours in the day 😢 ).

@johlju
Copy link
Contributor Author

johlju commented May 22, 2018

I see if I have time to start looking at this (probably during a weekend).

@PlagueHO
Copy link
Collaborator

I'll see if I have some time too - but I'm all my spare time is booked into doing some work on DFSDsc at the moment (one of the trickier ones to test because of the requirements for a AD Domain in most cases).

@johlju
Copy link
Contributor Author

johlju commented Jun 6, 2018

For reference. As per this dsccommunity/SqlServerDsc#135 (comment), if the releases should not contain tests then today we need to use the "harness-model" folder structure.
But maybe we can do that for this "joint-model" too by changing the release pipeline. 🤔

@PlagueHO
Copy link
Collaborator

PlagueHO commented Jul 7, 2018

Working on this now.

@PlagueHO
Copy link
Collaborator

PlagueHO commented Jul 7, 2018

I'm looking at moving all my repos away from the harness folder structure. It doesn't actually look like it was required. It actually came with the Wiki generation from SharePointDsc. I'll test it with a few repos first and if it works OK I'll do them all.

However, I'll leave SharePointDsc and OfficeOnlineServerDsc alone. But if we could get those moved off the harness model then this would allow us to dump it entirely.

@johlju
Copy link
Contributor Author

johlju commented Jul 7, 2018

I agree that the best is moving away from the harness folder structure (optional though) and instead make the default shared model better (including Wiki-generation). Having one model is much easier to maintain I think.
I'm seeing the potential to make better automated releases. at least to the point where it creates a Nuget package in the repository Releases, that can be manually deployed to the Gallery. For the time being we won't add the main PowerShell's team Gallery (encrypted) API key to the appveyor.yml.

SharePointDsc is also using that "Harness" folder structure so the Tests will not get published. See @kwirkykat dsccommunity/SqlServerDsc#135 (comment) about excluding the tests.

On one hand I like the idea of not publishing the tests, I understand that big SharePointDsc server farms don't need the tests on all the servers. Also, the tests need Git on the target node to work and the user might not have that on the target node. So tests in this case should be seen as only being used in the CI. On the other hand, having them on GitHub (and only in the CI) means the master branch soon have advanced and the tests no longer work for a published version, so they will eventually fail to test any published version other than the latest.
Personally, I have no problem publishing the tests with the release to the Gallery, I think it has more benefits. If the tests is undesirable on production servers then they could be remove from the module that is publish to the pull server (or similar)?

/cc @ykuijs @NikCharlebois @kwirkykat

@PlagueHO
Copy link
Collaborator

I'm of two minds here - I don't see a problem having the tests in the module published to the Gallery, but understand that they wouldn't be required or used on a production server.

I think rather than adopt the harness model it would have been a better to allow each module to control how they get published to the PS Gallery - e.g. some sort of options file in the repo itself.

@johlju
Copy link
Contributor Author

johlju commented Jul 14, 2018

That sounds like a great idea with an options file. We need to see how that fits into @kwirkykat’s release steps.

@PlagueHO
Copy link
Collaborator

Is it possible to even include the release automation in the DSCResources repo so that we can get visibility on what happens and potentially enhance it to reduce the need for things like the harness? Obviously not including the PowerShell Gallery keys 😁

@johlju
Copy link
Contributor Author

johlju commented Jul 14, 2018

Agree the steps should be visable in DscResources - I know Katie has written down the steps in an issue or PR comment in one of the repositories (in DSC resource Kit) when we in the community discussed “something” 😄. Wonder if we can find that comment and use as a starting point for any updated steps.

@johlju
Copy link
Contributor Author

johlju commented Jul 18, 2018

Spent a few minutes trying to find the comment that @kwirkykat detailed the steps she does when releasing - but finding that without having a good keyword to search for is hard. I will ask to see if we can get this documented or the code added to a public repo, or DscResources.

@ykuijs
Copy link
Contributor

ykuijs commented Jul 19, 2018

What would be the impact of this change?

I agree with the fact that we should not ship the tests in the package, because:

  • On production servers, personally I do not want the tests to be deployed. I only want the bare essentials: The resources.
  • If you need the tests to debug/troubleshoot, they are always available for download in the GitHub repo
  • Adding the tests will only increase the size of the package

@PlagueHO
Copy link
Collaborator

@ykuijs - IMHO a good way to do this would be to be able to exert some control over the packaging of the resource during the publish process. Perhaps by way of an publish options file. This could allow us to prevent certain folders (e.g. Tests or Docs) from being included in the deployment package -on a per repo basis. This would potentially allow us to no longer need the harness module structure (e.g. the manifest being nested several folders deep). For example, perhaps even adding a .publishignore file that allows us to specify folders/paths to exclude when creating the publish package.

However, for this to work it would require changes to the publish process that @kwirkykat uses.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement The issue is an enhancement request. help wanted The issue is up for grabs for anyone in the community.
Projects
Development

No branches or pull requests

3 participants