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

Improve GITHUB_DOMAIN/GITHUB_SERVER_URL default value handling #4422

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

Shegox
Copy link
Contributor

@Shegox Shegox commented Jul 3, 2023

Follow up to #4362
Inspired by #2031

Proposed Changes

  1. If GITHUB_DOMAIN exists, use its value as GitHub domain
  2. If GITHUB_DOMAIN does not exist, but GITHUB_SERVER_URL is already set, use its value as GitHub domain
  3. If neither GITHUB_DOMAIN nor GITHUB_SERVER_URL is set, use https://github.com as GitHub domain

Readiness Checklist

You can test the change with following configuration:

  • Update the docker image in the action.yml to ghcr.io/shegox/super-linter:github-domains-2 in your testing fork
  • Use it with [your-fork]/github-super-linter@[your-branch]

Testing with following settings in GitHub Actions:

  1. GITHUB_DOMAIN=https://github.enterprise.com
  2. GITHUB_DOMAIN=github.enterprise.com
  3. GITHUB_DOMAIN unset ✅

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

//cc @zkoppert

GITHUB_SERVER_URL="https://github.com"
fi
# Extract domain name from URL
GITHUB_SERVER_URL=$(echo "$GITHUB_SERVER_URL" | cut -d '/' -f 3)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is going to work with existing configurations. If I understand correctly, existing configurations $GITHUB_DOMAIN looks like mycompany.github.com so that then down on line 677, the https:// is added in.

Your pull request assumes that the user is inputting the https:// and if they don't it chomps up some of the url. ie. echo "zack.github.com/" | cut -d '/' -f 3 returns nothing because the cut has chopped off the who string.

Instead, this code should either detect if the https:// is present in the variable or as previous code did, it should assume it is not present. Does that make sense?

Copy link
Contributor Author

@Shegox Shegox Jul 4, 2023

Choose a reason for hiding this comment

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

thats correct, but this is the current behavior as well as I copied the logic from originally line 657 to the top of the script. This code works with both URLs starting with https:// (e.g. https://zack.github.com) as well as ones without any / (e.g. zack.github.com):
image

Only if you add a trailing slash it starts breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying. Looking at the code before and after again I think i missed that the first time.

@zkoppert zkoppert added the enhancement New feature or request label Jul 3, 2023
@zkoppert zkoppert merged commit c390133 into super-linter:main Jul 5, 2023
4 checks passed
@Shegox Shegox deleted the github-domain branch July 24, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants