Skip to content

Support modern Octopus features added since repo last regularly maintained #25

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 49 commits into
base: master
Choose a base branch
from
Open

Support modern Octopus features added since repo last regularly maintained #25

wants to merge 49 commits into from

Conversation

matthewmarchus
Copy link

Hello, @Suremaker!

Tyler Technologies greatly benefits from your project. I wanted to say thank-you for this project as it has really transformed our process. However, we did have to make some large augmentations which we want to give back to the OSS community. These generally include most functionality added to Octopus since the builder was last maintained and improve the flexibility of the project to deliver normalized YAML (and even split out inline scripts) to make onboarding onto a project driven by this configurator even easier.

…Name); specifying no project gets all projects
@Suremaker
Copy link
Owner

Hello @matthewmarchus ,

It is great to hear that your company have found OctopusProjectBuilder useful and thanks a lot for sharing back the improvements!
I will review them and merge in to create a new major version of the tool.

Thanks!

Copy link
Owner

@Suremaker Suremaker left a comment

Choose a reason for hiding this comment

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

Hello @matthewmarchus ,

I have finally had some time to review the changes in order to merge them in and release the new version of the library.

First of all, thank you for the contribution and sharing back the improvement!

I have reviewed the changes and they looks great. I have left a set of rather minor comments about the code itself.

What I noticed is that with your changes the existing project tests are failing and there is no tests for the new functionality. Could you please fix them and provide tests for the new features so I could release a high quality app for the community?

Thanks!

@@ -0,0 +1,28 @@
pipeline {
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove this file as it won't be used during the project build.

.DownloadModel(options.ProjectName);

await new YamlSystemModelRepository(_loggerFactory).Save(model, options.DefinitionsDir, async yaml =>
{
Copy link
Owner

Choose a reason for hiding this comment

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

Please extract the Normalisation logic out of the Program class to make DownloadDefinitions() easy to follow.

var machineFilterResource = (MachineFilterResource)resource;

var machineFilterResource = (MachineFilterResource) resource;
Copy link
Owner

Choose a reason for hiding this comment

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

Please cleanup the file.

@@ -25,7 +25,7 @@ public static void UpdateWith(this ReferenceCollection collection, IEnumerable<s
return new ElementReference(resource.Name);
});
}

Copy link
Owner

Choose a reason for hiding this comment

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

Similarly here, this change is unnecessary.

resource.EnvironmentIds = new ReferenceCollection(await Task.WhenAll(model.Environments.Select(async e => await repository.Environments.ResolveResourceId(e))));
//resource.UserRoleIds = new ReferenceCollection(await Task.WhenAll(model.UserRoles.Select(async ur => await repository.UserRoles.ResolveResourceId(ur))));
//resource.ProjectIds = new ReferenceCollection(await Task.WhenAll(model.Projects.Select(async p => await repository.Projects.ResolveResourceId(p))));
//resource.EnvironmentIds = new ReferenceCollection(await Task.WhenAll(model.Environments.Select(async e => await repository.Environments.ResolveResourceId(e))));
Copy link
Owner

Choose a reason for hiding this comment

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

What is the reason for commenting out this code and loosing the team mapping capability?

@@ -4,7 +4,7 @@
using Octopus.Client.Model;
using Octopus.Client.Repositories.Async;

namespace OctopusProjectBuilder.Uploader.Tests.Helpers
Copy link
Owner

Choose a reason for hiding this comment

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

The project follows the rule where namespace corresponds the project & folder structure.
Please revert the namespace changes.

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.

2 participants