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

Convert composite action to typescript #61

Merged
merged 45 commits into from
Oct 17, 2023

Conversation

snehapar9
Copy link
Contributor

@snehapar9 snehapar9 commented Sep 19, 2023

This change converts the existing composite action to a typescript based action so we can incorporate the private registry scenario.

@snehapar9 snehapar9 changed the title Convert docker action to typescript Convert composite action to typescript Sep 19, 2023
@snehapar9 snehapar9 marked this pull request as ready for review September 19, 2023 23:50
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/TelemetryHelper.ts Outdated Show resolved Hide resolved
Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

Some initial comments to take a look at

package.json Outdated Show resolved Hide resolved
azurecontainerapps.ts Outdated Show resolved Hide resolved
azurecontainerapps.ts Outdated Show resolved Hide resolved
azurecontainerapps.ts Outdated Show resolved Hide resolved
azurecontainerapps.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/TelemetryHelper.ts Outdated Show resolved Hide resolved
src/Utility.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
azurecontainerapps.ts Outdated Show resolved Hide resolved
Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

Last set of minor comments and then I think we're good 😃

azurecontainerapps.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerRegistryHelper.ts Outdated Show resolved Hide resolved
src/GithubActionsToolHelper.ts Outdated Show resolved Hide resolved
src/GithubActionsToolHelper.ts Outdated Show resolved Hide resolved
@daniv-msft
Copy link
Contributor

I was SURE I made this comment on the PR a couple of weeks back, and I cannot find it so I must have dreamt/forgot to publish it: is it right to assume that most of the code changes are exactly the same as the ADO Task, and that as such we should focus the review on what has changed? If yes, could you please point out the files that are new additions?

@snehapar9
Copy link
Contributor Author

snehapar9 commented Oct 12, 2023

I was SURE I made this comment on the PR a couple of weeks back, and I cannot find it so I must have dreamt/forgot to publish it: is it right to assume that most of the code changes are exactly the same as the ADO Task, and that as such we should focus the review on what has changed? If yes, could you please point out the files that are new additions?

From here all the files under src should be the same with some expections AzureAuthenticationHelper.ts is removed in Github Actions because it is not required while using the actions toolkit. GitHubActionsToolHelper.ts has been newly added to this repo. It contains functionality unique to just the Github Actions toolkit, the plan is to create a similar file in the Azure Devops Task to house functionalities unique to the Azure Devops lib.

The dist folder is a new addition but was created by compiling azurecontainerapps.ts and does not need to be reviewed.

src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
src/TelemetryHelper.ts Outdated Show resolved Hide resolved
src/ContainerRegistryHelper.ts Outdated Show resolved Hide resolved
src/ContainerAppHelper.ts Outdated Show resolved Hide resolved
@cormacpayne cormacpayne changed the base branch from main to v2-main October 13, 2023 22:25
Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@daniv-msft
Copy link
Contributor

I was SURE I made this comment on the PR a couple of weeks back, and I cannot find it so I must have dreamt/forgot to publish it: is it right to assume that most of the code changes are exactly the same as the ADO Task, and that as such we should focus the review on what has changed? If yes, could you please point out the files that are new additions?

From here all the files under src should be the same with some expections AzureAuthenticationHelper.ts is removed in Github Actions because it is not required while using the actions toolkit. GitHubActionsToolHelper.ts has been newly added to this repo. It contains functionality unique to just the Github Actions toolkit, the plan is to create a similar file in the Azure Devops Task to house functionalities unique to the Azure Devops lib.

The dist folder is a new addition but was created by compiling azurecontainerapps.ts and does not need to be reviewed.

Thanks @snehapar9! I'm reviewing specifically GitHubActionsToolHelper.ts in that case.

}

public getDefaultContainerAppName(containerAppName: string): string {
containerAppName = `gh-action-app-${this.getBuildId()}-${this.getBuildNumber()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: do we need both the buildId and buildNumber?

@snehapar9 snehapar9 marked this pull request as draft October 17, 2023 03:57
@snehapar9 snehapar9 marked this pull request as ready for review October 17, 2023 16:59
Comment on lines 29 to 32
// If a Container Registry URL was provided, try to authenticate against it
if (!this.util.isNullOrEmpty(this.registryUrl)) {
await this.authenticateContainerRegistryAsync();
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's move this block before the acrName check block above because authenticateAzureContainerRegistryAsync is going to set the value of registryUrl to include the value of acrName, so both of these if-statements will be hit

Copy link
Member

@cormacpayne cormacpayne left a comment

Choose a reason for hiding this comment

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

:shipit:

@snehapar9 snehapar9 merged commit 1c6a07a into v2-main Oct 17, 2023
1 check passed
@snehapar9 snehapar9 deleted the snehapar/container-apps-deploy-action-javascript branch October 17, 2023 20:35
cormacpayne added a commit that referenced this pull request Jan 9, 2024
* Convert composite action to typescript (#61)

* Convert docker action to typescript

* Add step to delete container app env back

* Remove workflow_dispatch

* Remove commented out code

* Fix formating

* Update image name

* Address PR comments

* Remove reference to Command Helper

* Address PR comments

* Remove commented code

* ci: add Azure Static Web Apps workflow file
on-behalf-of: @Azure [email protected]

* Delete .github/workflows/azure-static-web-apps-ashy-ocean-015179e0f.yml

* Update Azure CLI extensions installation

* Await setupAzureCli

* Refactor and trigger pipeline

* Refactor TelemetryHelper

* Reformat and remove new line

* Refactor and re-trigger pipeline

* Address PR comments

* Address PR comments

* Change comment for build-specific properties

* Refactor default containerapp name

* Change command construction

* Change command signature

* Fix command to get runtime stack

* Fix oryx dockerfile command to get runtime stack

* Fix oryx dockerfile command to get runtime stack

* Fix command to set default builder

* Test containerapp creation

* Hard code location and re-trigger pipeline

* Re-trigger pipeline and test

* Fix docker push command

* Address PR comments

* Fix formating for oryx dockerfile command

* Add double quotes

* Address PR comments

* Change executeAndThrowIfError to execute

* Support registry arguments

* Updated index.js

* Fix docker login command

* Re-order container registry authentication

* Add space

* Migrate latest changes from v1 to v2 (#68)

* Migrate latest changes from v1 to v2

* Support build arguments for Dockerfile and builder scenario

* Fix typo in docs

* Support private registry scenario (#69)

* Initial commit to support private registry scenario

* Add global var to delegate build to CLI

* Update condition to use CLI

* Fix formatting

* Update validation for internal registry scenario

* Address PR comments

* Update condition to use internal registry

* Test validation for internal registry scenario

* Get value of imageToBuild before build

* Fetch imageToBuild arg and test

* Move code to fetch imageToBuild in validation

* Install extension

* Trigger pipeline

* Re-trigger pipeline to install extensions

* Re-trigger pipeline to isntall extension

* Install extension and re-trigger pipeline

* Explicitly install extension

* Trigger pipeline

* Re-trigger pipeline

* Change whl name

* Fix pipeline failure and re-trigger

* Fix pipeline failures

* Install extension explicitly

* Fix valiation failures

* Fix validation failures

* Test validation failures

* Test validation failures

* Remove location

* Test validation pipeline

* Test validation pipeline

* Test validation pipeline

* Test validation pipeline

* Test validation pipeline

* Trigger validation pipeline

* Test validation pipeline

* Address validation failures

* Address PR discussion comments

* Made `imageToDeploy` optional

* Remove step to install whl

* Address PR comments

* Addressed PR comments

* Fix typo

* Remove space

* Fix merge failure

* Remove --debug

* Test registry scenario

* Fix merge failures and re-trigger pipeline

* Add --debug

* Create containerapp with up

* Create container app with up

* Trigger validation pipeline

* Test container app creation with up

* Add location to up command

* Use up for just private registry scenario

* Change rg for update scenario

* Remove step to delete pushed image

* Test containerapp update with up

* Hardcode location to northcentralusstage

* Remove app source path

---------

Co-authored-by: Cormac McCarthy <[email protected]>

* Remove env as a required arg (#72)

* Remove env as a required arg

* Add index.js

* Address PR comments

* Remove env param

* Update targeted builders to 20231107.2

* Build and push non cloud scenarios from GHA (#75)

* Snap to containerapp version 0.3.43

* Modify update command

* Comment out imageToDeploy

* Add --debug to the update command

* Comment out `--source`

* Enable build and push from ghs

* Build and push from gha

* Ensuer -i and --source are not used together

* Fix typo

* Revert comment

* Add global var to determine cloud build scenario

* Remove location (#76)

* Remove location

* Change logic to set location

* Check if env is provided

* Fix logic to set location

* Update logic to set location

* Add comments

* Remove space

* Remove location param

* Remove location

* Refactor

* Reorder arguments

* Address PR comments

* Fix typo

* Fix typo

* Fix PR comments

* Address PR comments

* Revert change to set this.containerrAppExists

* Refactor

* Add comment

* Change validation pipeline name

* support build environment variable

* fix comiple error

* build ts file

* package ts file

* Revert "fix comiple error"

This reverts commit 6cecedf.

* fix validation error

* Update Container App with 'containerapp update'
and 'ingress update' commands

* support quotation mark

* fix validation

* add log

* build typescript to js

* Revert "fix comiple error"

This reverts commit 6cecedf.

* package ts to js

* define constant for regex

* build ts to js

* Update Container App and Ingress separately

* Addressed comment and kept support for admin cred

* Fixed typo in validation

* support docker file build for buildArguments

* Update Azure login secrets for OIDC testing

* refine readme format

* add validation for build environment variable

* refine env name validation logic

* Fixed typo and updated timeout on validation task

* Fixed mismatch resource group name in validation

* regenerate index.js

* fix merge mistake

* add examples for arg buildArguments

* add usage for buildArguments

* not validate build argument name when there's Dockerfile in root path

---------

Co-authored-by: snehapar9 <[email protected]>
Co-authored-by: Dan Vouaux <[email protected]>
Co-authored-by: daniv-msft <[email protected]>
Co-authored-by: yilims <[email protected]>
Co-authored-by: harrli <[email protected]>
Co-authored-by: Harry Li <[email protected]>
Co-authored-by: Yi Li <[email protected]>
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