Skip to content

Support sync multiple files NGINX configuration to an NGINX for Azure deployment #6

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

Merged
merged 5 commits into from
May 16, 2022

Conversation

bangbingsyb
Copy link
Contributor

@bangbingsyb bangbingsyb commented May 12, 2022

The change enables the action to support sync multiple files NGINX config to NGINX for Azure by:

  1. Packaging the NGINX config directory in the Git repo to a tarball.
  2. Optionally, overwrite the paths of the config files in the directory on-the-fly during packaging based on a user provided "transformed" absolute path. This allows users to explicitly specify the "directory path" in NGINX for Azure deployment if the NGINX configuration files use absolute path in "include" directive.
  3. Upload the tarball to an NGINX for Azure deployment using ARM template deployment.

Challenges

  1. Git action workflow agents check out the repo source code in an arbitary working space directory and we don't have control on its path.
  2. Users might use either relative (to NGINX prefix path) or absolute paths for "include" directives in their existing NGINX config files.

Solution
To overcome the above challenges, file path transformation is required when pack the config directory. The implementation leverages two key features from tar: https://man7.org/linux/man-pages/man1/tar.1.html

  1. "-C": change the working directory of tar to the config directory before packaging. This ensures the paths in tarball are agnostic to the working space directory path used by the agent where the action will run.
  2. "--xform": tranform the file path based on sed-style expression during packaging. This enables file path transformation to support relative or absolute paths.

Action metadata changes
Introduce a new optional input parameter 'transformed-nginx-config-directory-path' to allow users to overwrite the absolute path of the config directory to the NGINX for Azure deployment. If not specified, relative paths to the config directory will be used for the files.

Testing
Testing has been done for a config directory containing child config and njs files in nested subdirectories:

  1. Using relative paths
    -- Config: https://github.com/bangbingsyb/nginx-config-github-composite-action/tree/main/config-relative-paths
    -- Action result: https://github.com/bangbingsyb/nginx-config-github-composite-action/actions/runs/2311611077
  2. Using absolute paths
    -- Config: https://github.com/bangbingsyb/nginx-config-github-composite-action/tree/main/config-absolute-paths
    -- Action result: https://github.com/bangbingsyb/nginx-config-github-composite-action/actions/runs/2311600337

@ornj
Copy link
Member

ornj commented May 12, 2022

While it's not the primary use case, it would be nice to document that this now requires gnu tar and the shell script will not work when run on OS X without brew install gnu-tar or running in a container.

@bangbingsyb
Copy link
Contributor Author

While it's not the primary use case, it would be nice to document that this now requires gnu tar and the shell script will not work when run on OS X without brew install gnu-tar or running in a container.

I see. Let me address OS compatibility in a follow up PR - I think multiple places need to be addressed to ensure such compatibility, for example: tar, uuid/random string generator. I don't have a mac and mostly tested the shell script from Ubuntu, Windows subsystem for Linux or Git bash for Windows (bash emulation). Will do some investigation to figure out a proper approach.

@agazeley
Copy link
Contributor

Are we completely removing single file support? I know it will not be the majority usecase and it is easy to understand that nginx configs should all be under one directory, but is there any desire to just support a single file config anywhere in the repo?

@bangbingsyb
Copy link
Contributor Author

bangbingsyb commented May 12, 2022

Are we completely removing single file support? I know it will not be the majority usecase and it is easy to understand that nginx configs should all be under one directory, but is there any desire to just support a single file config anywhere in the repo?

As long as the single file is in its own dedicated directory in the repo, the action will work properly. Multiple files support is a super set of single file support. I don't seem to see a strong reason why customer cannot put nginx config, even a single one, in its own directory, and instead must require the action to pick config from individual file paths. Thought?

@agazeley
Copy link
Contributor

Are we completely removing single file support? I know it will not be the majority usecase and it is easy to understand that nginx configs should all be under one directory, but is there any desire to just support a single file config anywhere in the repo?

As long as the single file is in its own dedicated directory in the repo, the action will work properly. Multiple files support is a super set of single file support. I don't seem to see a strong reason why customer cannot put nginx config, even a single one, in its own directory, and instead must require the action to pick config from individual file paths. Thought?

My only concern is if people are stuffing more than just the nginx configs in that directory. The config update will ignore the files in that case though so we should be good.

@bangbingsyb
Copy link
Contributor Author

bangbingsyb commented May 12, 2022

Are we completely removing single file support? I know it will not be the majority usecase and it is easy to understand that nginx configs should all be under one directory, but is there any desire to just support a single file config anywhere in the repo?

As long as the single file is in its own dedicated directory in the repo, the action will work properly. Multiple files support is a super set of single file support. I don't seem to see a strong reason why customer cannot put nginx config, even a single one, in its own directory, and instead must require the action to pick config from individual file paths. Thought?

My only concern is if people are stuffing more than just the nginx configs in that directory. The config update will ignore the files in that case though so we should be good.

Right. So the behavior shouldn't be broken. Besides that, consider the CICD scenario, this action should only be triggered if there is any NGINX config file change committed to the repo. All other changes are unrelated and should not trigger the action.

To properly configure such trigger for a GitHub workflow, it is also a better practice to have a dedicated folder with only NGINX configuration files inside. Trigger based on exact file name is not reliable upon file renaming. Then the workflow can be triggered upon any change to the files inside the folder using the "path" filter: https://github.com/bangbingsyb/nginx-config-github-composite-action/blob/main/.github/workflows/nginx-multi-files-config-absolute-paths.yml#L9

@bangbingsyb bangbingsyb merged commit 2ed0ce4 into main May 16, 2022
@bangbingsyb bangbingsyb deleted the bangbingsyb/multi-file-config branch May 16, 2022 23:38
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.

4 participants