You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/06/22 12:20:17 UTC

[GitHub] [airflow] potiuk opened a new pull request #16586: Switch to GitHub Container Registry by default

potiuk opened a new pull request #16586:
URL: https://github.com/apache/airflow/pull/16586


   Yesterday GitHub moved Github Container Registry to
   General Availability status. We are prepared to switch and tested
   it before, so this PR attempts to switch to it.
   
   <!--
   Thank you for contributing! Please make sure that your code changes
   are covered with tests. And in case of new features or big changes
   remember to adjust the documentation.
   
   Feel free to ping committers for the review!
   
   In case of existing issue, reference it using one of the following:
   
   closes: #ISSUE
   related: #ISSUE
   
   How to write a good git commit message:
   http://chris.beams.io/posts/git-commit/
   -->
   
   ---
   **^ Add meaningful description above**
   
   Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on a change in pull request #16586: Switch to GitHub Container Registry by default

Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #16586:
URL: https://github.com/apache/airflow/pull/16586#discussion_r656201482



##########
File path: scripts/ci/libraries/_build_images.sh
##########
@@ -422,30 +422,11 @@ function build_images::get_docker_image_names() {
 }
 
 # If GitHub Registry is used, login to the registry using GITHUB_USERNAME and
-# either GITHUB_TOKEN or CONTAINER_REGISTRY_TOKEN depending on the registry.
-# In case Personal Access token is not set, skip logging in
+# GITHUB_TOKEN. In case Personal Access token is not set, skip logging in
 # Also enable experimental features of docker (we need `docker manifest` command)
 function build_images::configure_docker_registry() {
     if [[ ${USE_GITHUB_REGISTRY} == "true" ]]; then
-        local token=""
-        if [[ "${GITHUB_REGISTRY}" == "ghcr.io" ]]; then
-            # For now ghcr.io can only authenticate using Personal Access Token with package access scope.
-            # There are plans to implement GITHUB_TOKEN authentication but this is not implemented yet
-            token="${CONTAINER_REGISTRY_TOKEN=}"
-            verbosity::print_info
-            verbosity::print_info "Using CONTAINER_REGISTRY_TOKEN!"
-            verbosity::print_info
-        elif [[ "${GITHUB_REGISTRY}" == "docker.pkg.github.com" ]]; then
-            token="${GITHUB_TOKEN}"
-            verbosity::print_info
-            verbosity::print_info "Using GITHUB_TOKEN!"
-            verbosity::print_info
-        else
-            echo
-            echo  "${COLOR_RED}ERROR: Bad value of '${GITHUB_REGISTRY}'. Should be either 'ghcr.io' or 'docker.pkg.github.com'!${COLOR_RESET}"
-            echo
-            exit 1
-        fi
+        local token="${GITHUB_TOKEN}"

Review comment:
       This is because GitHub added authentication via GITHUB_TOKEN in the meantime, so we do not need to configure the secret personal token any more.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #16586: Switch to GitHub Container Registry by default

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16586:
URL: https://github.com/apache/airflow/pull/16586#issuecomment-865955972


   Hey @ashb @kaxil - I think we should merge this on. It works https://github.com/potiuk/airflow/actions/runs/960600525 and the new "Github Container Registry" is much more stable (and forward-supported). We can always switch back (I have not removed the code for the deprecated package registry - I will eventually when we run it for couple of days.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk edited a comment on pull request #16586: Switch to GitHub Container Registry by default

Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #16586:
URL: https://github.com/apache/airflow/pull/16586#issuecomment-865955972


   Hey @ashb @kaxil - I think we should merge this one. It works https://github.com/potiuk/airflow/actions/runs/960600525 and the new "Github Container Registry" is much more stable (and forward-supported). We can always switch back (I have not removed the code for the deprecated package registry - I will eventually when we run it for couple of days.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk commented on pull request #16586: Switch to GitHub Container Registry by default

Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #16586:
URL: https://github.com/apache/airflow/pull/16586#issuecomment-865959272


   Currently most of the builds fails on random timeout of logging in (like one of the jobsa above) - it failed because it still uses the old registry until we merge it to main. This PR is all green so far in my fork when run as main https://github.com/potiuk/airflow/actions/runs/960600525


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [airflow] potiuk merged pull request #16586: Switch to GitHub Container Registry by default

Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #16586:
URL: https://github.com/apache/airflow/pull/16586


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org