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

Added support for Azure Batch #1

Merged
merged 1 commit into from
May 17, 2024
Merged

Added support for Azure Batch #1

merged 1 commit into from
May 17, 2024

Conversation

WhitWaldo
Copy link
Contributor

Someone on the Aspire team pointed me towards your project. While I've spent a little time thinking of alternatives to their approach, I read through how your project is put together and it's a much more progressed version of what I was doing, so I hope you don't mind me jumping in with some additional resources.

I thought I'd start with Batch since it's a relatively simple resource, it's something my own project has a dependency on and it's something that's probably not doing to be added to Aspire in the visible future. I also added certificates on the Batch accounts so I could try out your approach for parented resources.

Because you already had a validation method, I added a string extension to assist with naming validation and an enum extension that helps with retrieving the expected strings for some of these enums (as some of them, like AllowedAuthenticationMode have legacy values that wouldn't make since with the modern product names). And of course, unit tests similar to those you've already got in place.

There's more that could be done here (pools, for example), but they're a bit more involved, so I just wanted to start somewhere.

Eager to jump in and contribute more resources if you're open to it, and see about helping out with some of the plumbing.

@rudiv
Copy link
Owner

rudiv commented May 17, 2024

Thanks for the contribution @WhitWaldo, much appreciated!

I'm working on adding in more support as and when we're needing it for our own projects, so external contributions for more resources are certainly appreciated. I'll get this reviewed properly and merged in hopefully this weekend 👍

SyntaxFactory.DotToken,
null,
SyntaxFactory.CreateIdentifier(splitPath[1]));
SyntaxFactory.CreateIdentifier(second));
Copy link
Owner

Choose a reason for hiding this comment

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

Approving this - but as a note for the future this should probably throw an error? It's not a case that it will ever hit (hopefully) as a property access requires at least 2 parts, but just to be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's definitely a better way to do this, but I just wanted to get something out there that worked for my new component while also not breaking what was there. There's a lot of opportunity for some helper methods like this in the plumbing as I get more familiar with it all.

Copy link
Owner

Choose a reason for hiding this comment

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

Agree - I haven't posted about this library much at all yet until there's more resources in here, though I do think it's super valuable for a) support of more resources that Aspire probably won't have support for and b) configuring things "properly" for a production environment.

If it gets more traction, will definitely clean things up more 🙂

@rudiv rudiv merged commit 30ac6c8 into rudiv:main May 17, 2024
1 check passed
@rudiv
Copy link
Owner

rudiv commented May 17, 2024

Thank you again @WhitWaldo - very much appreciated. Pushing out as 0.2.5 now.

If there's anything else you'd like to see above the already planned resource please let me know 🙏 I should get chance to get more in during the coming weeks.

@WhitWaldo WhitWaldo deleted the azure-batch branch May 17, 2024 23:51
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