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

[ODS-6435] Use preinstalled Microsoft SQL Server in Rebuild Database Templates.yml #1135

Merged
merged 11 commits into from
Sep 12, 2024

Conversation

mjaksn
Copy link
Contributor

@mjaksn mjaksn commented Sep 10, 2024

No description provided.

@mjaksn mjaksn marked this pull request as ready for review September 10, 2024 04:27
@mjaksn mjaksn requested a review from a team as a code owner September 10, 2024 04:28
run: git config --system core.longpaths true
- name: Set permissions for database server container backup directory
run: docker exec --user root mssql chmod 777 /DatabaseBackups
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

Copy link
Contributor Author

@mjaksn mjaksn Sep 11, 2024

Choose a reason for hiding this comment

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

@semalaiappan , can you elaborate on the reasoning for changing bash to pwsh? Bash is the default shell on Ubuntu, and the steps where this change is requested are all intended to execute a binary on the Ubuntu host. The "Set permissions for database server container backup directory" steps directly invoke the docker executable on the runner host, and the "Set permissions for database backup file" steps execute a Linux-specific command (chmod), which modifies permissions on the Ubuntu host's file system in a way without a direct equivalent in windows or PowerShell. I don't believe the change to pwsh would cause any harm. However, I hesitate to use pwsh if its unique capabilities are not required. It appears that for these steps, it would only act as an additional layer that the one-line commands must pass through to run in the underlying Ubuntu environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your point . we are always using pwsh script in all steps in github actions workflows . That's the reason . Thank you !

@vimayya Please advice .

Copy link
Contributor

Choose a reason for hiding this comment

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

This specific command would run equally well in Bash or PowerShell.

Personally, I still use Bash commands sometimes, just for practice, but in general I prefer to have PowerShell commands in our GitHub runner steps. Since most of us are running Windows computers, having PowerShell commands in the Actions workflows makes it easier for us to manually test those commands locally.

- name: Set permissions for database backup file
working-directory: ./
run: sudo chmod -R 777 $GITHUB_WORKSPACE/Ed-Fi-ODS-Implementation/DatabaseTemplate/Database/
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

run: git config --system core.longpaths true
- name: Set permissions for database server container backup directory
run: docker exec --user root mssql chmod 777 /DatabaseBackups
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

- name: Set permissions for database backup file
working-directory: ./
run: sudo chmod -R 777 $GITHUB_WORKSPACE/Ed-Fi-ODS-Implementation/DatabaseTemplate/Database/
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

run: git config --system core.longpaths true
- name: Set permissions for database server container backup directory
run: docker exec --user root mssql chmod 777 /DatabaseBackups
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

- name: Set permissions for database backup file
working-directory: ./
run: sudo chmod -R 777 $GITHUB_WORKSPACE/Ed-Fi-ODS-Implementation/DatabaseTemplate/Database/
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

run: git config --system core.longpaths true
- name: Set permissions for database server container backup directory
run: docker exec --user root mssql chmod 777 /DatabaseBackups
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

- name: Set permissions for database backup file
working-directory: ./
run: sudo chmod -R 777 $GITHUB_WORKSPACE/Ed-Fi-ODS-Implementation/DatabaseTemplate/Database/
shell: bash
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change it to pwsh instead of bash ?

@semalaiappan semalaiappan merged commit 8a3ef49 into main Sep 12, 2024
41 checks passed
@semalaiappan semalaiappan deleted the ODS-6435 branch September 12, 2024 16:54
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.

3 participants