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

Make x509 certificate script from azure-sdk-for-net common to repos #6543

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

benbp
Copy link
Member

@benbp benbp commented Jul 19, 2023

Copying from https://github.com/Azure/azure-sdk-for-net/blob/main/eng/scripts/X509Certificate2/X509Certificate2.psm1 so that we can share cert generating functionality.

Thanks for the suggestion @conniey

@benbp benbp added the Central-EngSys This issue is owned by the Engineering System team. label Jul 19, 2023
@benbp benbp self-assigned this Jul 19, 2023
@benbp benbp requested review from heaths and christothes July 19, 2023 20:39
Copy link
Member

@conniey conniey left a comment

Choose a reason for hiding this comment

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

🥂 lgtm

@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

Usage:

```powershell
Import-Module -Name eng/scripts/X509Certificate2.psm1 # assumes $PWD is repo root
Copy link
Member

@weshaggard weshaggard Jul 19, 2023

Choose a reason for hiding this comment

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

Does this have to be a module? In general we have moved away from modules do to loading issues and just use a script with helper functions. However I guess it was already developed that way and you are just porting it to common so we can leave it as is.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was developed that way, but I'm also happy to spend a few minutes turning it into a cmdlet instead.

Copy link
Member

Choose a reason for hiding this comment

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

A cmdlet is a module right? At any rate I'm OK with this for now and if we hit any issues we can reevaluate.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think of top-level scripts as cmdlets

Copy link
Member

Choose a reason for hiding this comment

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

Modules contain 1 or more cmdlets (or none, actually; could just be type or view data). Scripts can be cmdlets. Modules can be unloaded, and because these are all a bunch of related cmdlets I didn't want a bunch of one-off scripts that are harder to keep in sync. In my experience, I've never had problems with modules. They solved the fundamental problems with snapins.

Copy link
Member

Choose a reason for hiding this comment

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

We have definitely had issues with module loading and unloading, especially during development of them as you often times need to restart your shell to unload successfully. We also have issues when loading modules in parallel when running steps which causes race conditions and other interesting things. That is why we moved to having a set of shared helper scripts/functions instead of modules. That said if I don't have concerns with keeping this as a module if it is already working well as one.

@christothes
Copy link
Member

Is there any plan to delete the .net repo script that this replaces, or work items to track it?

@benbp
Copy link
Member Author

benbp commented Jul 20, 2023

Is there any plan to delete the .net repo script that this replaces, or work items to track it?

@christothes yes after this is merged I was going to delete the NET repo script. Is this part of people's workflows and/or other documentation that I should update? (e.g. should I leave a redirect note in the net/eng directory, or can I just delete it entirely)

@christothes
Copy link
Member

christothes commented Jul 20, 2023

@christothes yes after this is merged I was going to delete the NET repo script. Is this part of people's workflows and/or other documentation that I should update? (e.g. should I leave a redirect note in the net/eng directory, or can I just delete it entirely)

Thanks! - here are the ones I am aware of

@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

@benbp
Copy link
Member Author

benbp commented Jul 24, 2023

@christothes PR for net here. Not all the references are changed because a few services have implemented their own function with the same name.

@benbp benbp force-pushed the benbp/cert-scripts branch from 63d0c0f to 1bc4c3c Compare July 24, 2023 19:30
@azure-sdk
Copy link
Collaborator

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

azure-sdk added a commit to Azure/azure-sdk-for-js that referenced this pull request Jul 24, 2023
@benbp benbp merged commit d322131 into Azure:main Jul 24, 2023
@benbp benbp deleted the benbp/cert-scripts branch July 24, 2023 20:52
dgetu pushed a commit to Azure/azure-sdk-for-js that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Central-EngSys This issue is owned by the Engineering System team.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants