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.

Sign up for GitHub

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

Jump to bottom

Ag common files #2419

Merged
merged 21 commits into from
Feb 15, 2024
Merged

Ag common files #2419

merged 21 commits into from
Feb 15, 2024

Conversation

sebassem
Copy link
Contributor

  • Consolidating the artifacts codebase for Agora scenarios

@sebassem
Merge branch 'microsoft:main' into ag_common_files
30e6b3d
@sebassem
Add new icons and settings files, delete old icons, and add data emul…
1ac1af8 
…ator and store JSON file
@sebassem
Update templateBaseUrl to include industry parameter
fd0a31a
@sebassem
Commented out downloading of AgConfig.psd1 file
0a018ae
@sebassem
Fix file path in Bootstrap.ps1
431fed1
@sebassem
Fix GitHub API URL in AgLogonScript.ps1
64694fd
@sebassem
Add subscription ID to Azure Logon Script
a2b559d
@sebassem
Add new files and update existing files
c3324f2
@sebassem
Update user authentication logic***
430d738
@sebassem
Fix bug in login functionality***
969dfe9 
***Refactor code for better performance***

***Update UI layout for improved user experience***

***Add new feature for user authentication***

***Fix typo in variable name***

***Remove unused imports***

***Update documentation for new API endpoints***

***Optimize database queries for faster response times***

***Fix issue with error handling in API calls***

***Add unit tests for new functionality
@sebassem
Add manufacturing-specific settings and mq_cloudConnector.yml
ddee32a
@sebassem
formatting
7cb08c4
@sebassem
Add industry parameter to client VM deployment
ce36e9c
@sebassem
Update templateBaseUrl in main.azd.bicep and main.bicep
5a1b62a
@sebassem
Add industry parameter to retail and manufacturing deployments
b047e5f
@sebassem
Update industry parameter value to 'manufacturing'
19451b5
@sebassem
Update user authentication logic
b99eb82
@sebassem
Update githubBranch parameter value in main.azd.bicep
62808ca
@sebassem
Update githubBranch parameter value in main.bicep
12714ea
@sebassem
Merge branch 'microsoft:main' into ag_common_files
01988c6
@sebassem sebassem requested a review from likamrat February 14, 2024 14:53
@microsoft-github-policy-service Microsoft GitHub Policy Service
Copy link

Hi sebassem! Thank you for opening this Pull Request. Someone will review it soon. Thank you for committing to making the Azure Arc Jumpstart better.

@sebassem sebassem requested a review from dkirby-ms February 14, 2024 16:36
dkirby-ms
dkirby-ms reviewed Feb 14, 2024
View reviewed changes
Copy link
Contributor

@dkirby-ms dkirby-ms left a comment

Choose a reason for hiding this comment

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

Some general feedback on code layout and some specifics around paths and logging. We can merge and work on these moving forward rather than being a blocker right now.

I also see some HCIBox related files in here so I think you need to fetch latest into your fork before merge.

azure_jumpstart_ag/artifacts/PowerShell/AgLogonScript.ps1
$AgConfig = Import-PowerShellDataFile -Path $Env:AgConfigPath
$AgToolsDir = $AgConfig.AgDirectories["AgToolsDir"]
$AgIconsDir = $AgConfig.AgDirectories["AgIconDir"]
$AgAppsRepo = $AgConfig.AgDirectories["AgAppsRepo"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider referncing folder paths via a hashtable instead of via array lookup.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Its easier to read IMO

azure_jumpstart_ag/artifacts/PowerShell/AgLogonScript.ps1


Start-Transcript -Path ($AgConfig.AgDirectories["AgLogsDir"] + "\AgLogonScript.log")
Write-Header "Executing Jumpstart Agora automation scripts"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should drop use of Write-Header and the PS1Profile injection where it gets defined and just stick with Write-Host. For context, I have removed Write-Header completely for HCIBox.

azure_jumpstart_ag/artifacts/PowerShell/AgLogonScript.ps1
Copy link
Contributor

Choose a reason for hiding this comment

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

The length of this script seems like it might be better suited broken into defined functions. For example:

  • Configure-L1Virtualization
  • Configure-AzureIoT
  • Configure-Kubernetes

And in each of these we are doing the heavy lifting that is being done in the main thread right now.

@sebassem sebassem merged commit f015223 into microsoft:ag_manufacturing Feb 15, 2024
1 check passed
@sebassem sebassem deleted the ag_common_files branch February 15, 2024 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dkirby-ms dkirby-ms dkirby-ms left review comments

@likamrat likamrat Awaiting requested review from likamrat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sebassem @dkirby-ms