You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/10/27 21:09:59 UTC
[GitHub] [trafficcontrol] ocket8888 opened a new pull request #5209: Perlectomy
ocket8888 opened a new pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209
## What does this PR (Pull Request) do?
- [x] This PR is not related to any Issue
This PR removes the Traffic Ops Perl codebase. It does _not_ remove API v1 from the Go server.
## Which Traffic Control components are affected by this PR?
- CDN in a Box
- Documentation
- Traffic Ops
## What is the best way to verify this PR?
Make sure the tests pass, probably.
## The following criteria are ALL met by this PR
- [ ] This PR includes tests OR I have explained why tests are unnecessary
- [ ] This PR includes documentation OR I have explained why documentation is unnecessary
- [ ] This PR includes an update to CHANGELOG.md OR such an update is not necessary
- [ ] This PR includes any and all required license headers
- [ ] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
- [ ] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578041921
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -20,129 +20,76 @@
# Based on CentOS 8
############################################################
-# Keep the trafficops-common-deps in Dockerfile the same as
-# trafficops-common-deps in Dockerfile-go to cache the same
-# layer.
ARG RHEL_VERSION=8
-FROM centos:${RHEL_VERSION} as trafficops-common-deps
+FROM centos:${RHEL_VERSION}
ARG RHEL_VERSION=8
# Makes RHEL_VERSION available in later layers without needing to specify it again
ENV RHEL_VERSION=$RHEL_VERSION
RUN if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- yum -y install dnf || exit 1; \
- fi
+ yum -y install dnf || exit 1; \
+ fi
RUN set -o nounset -o errexit && \
- mkdir -p /etc/cron.d; \
- if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- use_repo=''; \
- else \
- use_repo='--repo=pgdg96'; \
- fi; \
- dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
- dnf -y $use_repo -- install postgresql96; \
- dnf -y install epel-release; \
- dnf -y install \
- jq \
- bind-utils \
- net-tools \
- gettext \
- perl-JSON-PP \
- mkisofs \
- isomd5sum \
- nmap-ncat \
- openssl \
- # Used to copy certs in "Shared SSL certificate generation" step
- rsync; \
- dnf clean all
-
-FROM trafficops-common-deps as trafficops-perl-deps
+ mkdir -p /etc/cron.d; \
+ if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
+ use_repo=''; \
+ else \
+ use_repo='--repo=pgdg96'; \
+ fi; \
+ dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
+ dnf -y $use_repo -- install postgresql96; \
+ dnf -y install epel-release; \
+ dnf -y install \
+ jq \
+ bind-utils \
+ net-tools \
+ gettext \
+ perl-JSON-PP \
+ perl-Crypt-ScryptKDF \
+ mkisofs \
+ isomd5sum \
+ nmap-ncat \
+ openssl \
+ python3 \
+ golang \
+ git \
+ # Used to copy certs in "Shared SSL certificate generation" step
+ rsync; \
+ dnf clean all
EXPOSE 443
-ENV MOJO_MODE production
-
-RUN if [[ "${RHEL_VERSION%%.*}" -ge 8 ]]; then \
- # If you get "Unknown repo: 'powertools'", pull a newer centos:8 image
- enable_repo='--enablerepo=powertools' || exit 1; \
- fi && \
- dnf -y --allowerasing $enable_repo install \
- cpanminus \
- expat-devel \
- gcc-c++ \
- git \
- golang \
- iproute \
- jq \
- libcurl \
- libcurl-devel \
- libidn-devel \
- libpcap-devel \
- openssl-devel \
- perl-core \
- perl-Crypt-ScryptKDF\
- perl-DBD-Pg \
- perl-DBI \
- perl-DBIx-Connector \
- perl-Digest-SHA1 \
- perl-JSON \
- perl-libwww-perl \
- perl-Net-Pcap \
- perl-TermReadKey \
- perl-Test-CPAN-Meta \
- perl-WWW-Curl \
- postgresql96-devel \
- tar && \
- dnf -y clean all && \
- mkdir -p /opt/traffic_ops/app/public && \
- cpanm Carton
ADD traffic_router/core/src/test/resources/geo/GeoLite2-City.mmdb.gz /opt/traffic_ops/app/public/
WORKDIR /opt/traffic_ops/app
-ADD traffic_ops/app/cpanfile traffic_ops/install/bin/install_goose.sh ./
+ADD traffic_ops/install/bin/install_goose.sh ./
+RUN ./install_goose.sh && rm ./install_goose.sh && dnf -y remove git && dnf clean all
-# Start with the existing traffic_ops/app/local directory
-COPY infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-${RHEL_VERSION:-8}/local local
-RUN export POSTGRES_HOME=/usr/pgsql-9.6 PERL5LIB=$(pwd)/local/lib/perl5 && \
- echo 'Running carton...' && \
- [[ -d "${PERL5LIB}/x86_64-linux-thread-multi" ]]; existing_install=$? && \
- if ! carton && [[ $existing_install -eq 0 ]]; then \
- rm -rf local && \
- echo 'Removed modules, retrying carton...' && \
- carton || exit 1; \
- fi && \
- rm -rf $HOME/.cpan* /tmp/Dockerfile /tmp/local.tar.gz ./cpanfile
-RUN ./install_goose.sh
+ADD infrastructure/cdn-in-a-box/traffic_ops_data /traffic_ops_data
-FROM trafficops-perl-deps
# Override TRAFFIC_OPS_RPM arg to use a different one using --build-arg TRAFFIC_OPS_RPM=... Can be local file or http://...
+#
ARG TRAFFIC_OPS_RPM=infrastructure/cdn-in-a-box/traffic_ops/traffic_ops.rpm
-ADD $TRAFFIC_OPS_RPM /
-RUN rpm -Uvh /$(basename $TRAFFIC_OPS_RPM) && \
- rm /$(basename $TRAFFIC_OPS_RPM) && \
- rm /opt/traffic_ops/app/bin/traffic_ops_golang
-
-# Run carton again, in case the cpanfile included in the RPM differs from the one used earlier in the
-# build (should never happen - Perl is supposed to be going away)
-RUN POSTGRES_HOME=/usr/pgsql-9.6 PERL5LIB=$(pwd)/local/lib/perl5 carton && \
- rm -rf $HOME/.cpan* /tmp/Dockerfile /tmp/local.tar.gz
-ADD infrastructure/cdn-in-a-box/enroller/server_template.json \
- infrastructure/cdn-in-a-box/traffic_ops/run.sh \
- infrastructure/cdn-in-a-box/traffic_ops/config.sh \
- infrastructure/cdn-in-a-box/traffic_ops/adduser.pl \
- infrastructure/cdn-in-a-box/traffic_ops/to-access.sh \
- infrastructure/cdn-in-a-box/traffic_ops/generate-certs.sh \
- infrastructure/cdn-in-a-box/traffic_ops/trafficops-init.sh \
- infrastructure/cdn-in-a-box/variables.env \
- /
+COPY $TRAFFIC_OPS_RPM /traffic_ops.rpm
+RUN rpm --install --nodeps --verbose --hash /traffic_ops.rpm && \
Review comment:
With `--nodeps` removed, there are unmet dependencies for both CentOS 7 and CentOS 8.
Dependencies that CentOS 7 complains about but CentOS 8 does not:
```shell
golang >= 1.12 is needed by traffic_ops-5.1.0-11232.dc08115a.
openssl-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
```
Dependencies that CentOS 8 complains about but CentOS 7 does not:
```shell
perl is needed by traffic_ops-5.1.0-11232.dc08115a.
```
---
The full lists:
With `RHEL_VERSION=7` in the environment and a CentOS 7 Traffic Ops RPM:
```shell
=> ERROR [11/12] RUN rpm --install --verbose --hash /traffic_ops.rpm && rm /traffic_ops.rpm 1.0s
------
> [11/12] RUN rpm --install --verbose --hash /traffic_ops.rpm && rm /traffic_ops.rpm:
error: Failed dependencies:
cpanminus is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
expat-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
gcc-c++ is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
golang >= 1.12 is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
libcurl-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
libidn-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
libpcap-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
openssl-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-DBD-Pg is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-DBI is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-Digest-SHA1 is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-JSON is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-TermReadKey is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-Test-CPAN-Meta is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-WWW-Curl is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-core is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
perl-libwww-perl is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
postgresql96-devel >= 9.6.2 is needed by traffic_ops-5.1.0-11232.dc08115a.el7.x86_64
```
With `RHEL_VERSION=8` in the environment and a CentOS 8 Traffic Ops RPM:
```
> [11/12] RUN rpm --install --verbose --hash /traffic_ops.rpm && rm /traffic_ops.rpm:
error: Failed dependencies:
cpanminus is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
expat-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
gcc-c++ is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
libcurl-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
libidn-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
libpcap-devel is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-DBD-Pg is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-DBI is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-Digest-SHA1 is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-JSON is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-TermReadKey is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-Test-CPAN-Meta is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-WWW-Curl is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-core is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
perl-libwww-perl is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
postgresql96-devel >= 9.6.2 is needed by traffic_ops-5.1.0-11232.dc08115a.el8.x86_64
```
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r576399132
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
We could send out an email on the users mailing list, see if anybody responds in like a week
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575509027
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
Is there a way we can confirm whether these scripts are really in use?
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578034858
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -20,129 +20,76 @@
# Based on CentOS 8
############################################################
-# Keep the trafficops-common-deps in Dockerfile the same as
-# trafficops-common-deps in Dockerfile-go to cache the same
-# layer.
ARG RHEL_VERSION=8
-FROM centos:${RHEL_VERSION} as trafficops-common-deps
+FROM centos:${RHEL_VERSION}
ARG RHEL_VERSION=8
# Makes RHEL_VERSION available in later layers without needing to specify it again
ENV RHEL_VERSION=$RHEL_VERSION
RUN if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- yum -y install dnf || exit 1; \
- fi
+ yum -y install dnf || exit 1; \
+ fi
RUN set -o nounset -o errexit && \
- mkdir -p /etc/cron.d; \
- if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- use_repo=''; \
- else \
- use_repo='--repo=pgdg96'; \
- fi; \
- dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
- dnf -y $use_repo -- install postgresql96; \
- dnf -y install epel-release; \
- dnf -y install \
- jq \
- bind-utils \
- net-tools \
- gettext \
- perl-JSON-PP \
- mkisofs \
- isomd5sum \
- nmap-ncat \
- openssl \
- # Used to copy certs in "Shared SSL certificate generation" step
- rsync; \
- dnf clean all
-
-FROM trafficops-common-deps as trafficops-perl-deps
+ mkdir -p /etc/cron.d; \
+ if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
+ use_repo=''; \
+ else \
+ use_repo='--repo=pgdg96'; \
+ fi; \
+ dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
+ dnf -y $use_repo -- install postgresql96; \
+ dnf -y install epel-release; \
+ dnf -y install \
+ jq \
+ bind-utils \
+ net-tools \
+ gettext \
+ perl-JSON-PP \
+ perl-Crypt-ScryptKDF \
+ mkisofs \
+ isomd5sum \
+ nmap-ncat \
+ openssl \
+ python3 \
+ golang \
+ git \
+ # Used to copy certs in "Shared SSL certificate generation" step
+ rsync; \
+ dnf clean all
EXPOSE 443
-ENV MOJO_MODE production
-
-RUN if [[ "${RHEL_VERSION%%.*}" -ge 8 ]]; then \
- # If you get "Unknown repo: 'powertools'", pull a newer centos:8 image
- enable_repo='--enablerepo=powertools' || exit 1; \
- fi && \
- dnf -y --allowerasing $enable_repo install \
- cpanminus \
- expat-devel \
- gcc-c++ \
- git \
- golang \
- iproute \
- jq \
- libcurl \
- libcurl-devel \
- libidn-devel \
- libpcap-devel \
- openssl-devel \
- perl-core \
- perl-Crypt-ScryptKDF\
- perl-DBD-Pg \
- perl-DBI \
- perl-DBIx-Connector \
- perl-Digest-SHA1 \
- perl-JSON \
- perl-libwww-perl \
- perl-Net-Pcap \
- perl-TermReadKey \
- perl-Test-CPAN-Meta \
- perl-WWW-Curl \
- postgresql96-devel \
- tar && \
- dnf -y clean all && \
- mkdir -p /opt/traffic_ops/app/public && \
- cpanm Carton
ADD traffic_router/core/src/test/resources/geo/GeoLite2-City.mmdb.gz /opt/traffic_ops/app/public/
WORKDIR /opt/traffic_ops/app
-ADD traffic_ops/app/cpanfile traffic_ops/install/bin/install_goose.sh ./
+ADD traffic_ops/install/bin/install_goose.sh ./
+RUN ./install_goose.sh && rm ./install_goose.sh && dnf -y remove git && dnf clean all
-# Start with the existing traffic_ops/app/local directory
-COPY infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-${RHEL_VERSION:-8}/local local
-RUN export POSTGRES_HOME=/usr/pgsql-9.6 PERL5LIB=$(pwd)/local/lib/perl5 && \
- echo 'Running carton...' && \
- [[ -d "${PERL5LIB}/x86_64-linux-thread-multi" ]]; existing_install=$? && \
- if ! carton && [[ $existing_install -eq 0 ]]; then \
- rm -rf local && \
- echo 'Removed modules, retrying carton...' && \
- carton || exit 1; \
- fi && \
- rm -rf $HOME/.cpan* /tmp/Dockerfile /tmp/local.tar.gz ./cpanfile
-RUN ./install_goose.sh
+ADD infrastructure/cdn-in-a-box/traffic_ops_data /traffic_ops_data
-FROM trafficops-perl-deps
# Override TRAFFIC_OPS_RPM arg to use a different one using --build-arg TRAFFIC_OPS_RPM=... Can be local file or http://...
+#
ARG TRAFFIC_OPS_RPM=infrastructure/cdn-in-a-box/traffic_ops/traffic_ops.rpm
-ADD $TRAFFIC_OPS_RPM /
-RUN rpm -Uvh /$(basename $TRAFFIC_OPS_RPM) && \
- rm /$(basename $TRAFFIC_OPS_RPM) && \
- rm /opt/traffic_ops/app/bin/traffic_ops_golang
-
-# Run carton again, in case the cpanfile included in the RPM differs from the one used earlier in the
-# build (should never happen - Perl is supposed to be going away)
-RUN POSTGRES_HOME=/usr/pgsql-9.6 PERL5LIB=$(pwd)/local/lib/perl5 carton && \
- rm -rf $HOME/.cpan* /tmp/Dockerfile /tmp/local.tar.gz
-ADD infrastructure/cdn-in-a-box/enroller/server_template.json \
- infrastructure/cdn-in-a-box/traffic_ops/run.sh \
- infrastructure/cdn-in-a-box/traffic_ops/config.sh \
- infrastructure/cdn-in-a-box/traffic_ops/adduser.pl \
- infrastructure/cdn-in-a-box/traffic_ops/to-access.sh \
- infrastructure/cdn-in-a-box/traffic_ops/generate-certs.sh \
- infrastructure/cdn-in-a-box/traffic_ops/trafficops-init.sh \
- infrastructure/cdn-in-a-box/variables.env \
- /
+COPY $TRAFFIC_OPS_RPM /traffic_ops.rpm
+RUN rpm --install --nodeps --verbose --hash /traffic_ops.rpm && \
Review comment:
`--nodeps` was in here for the `trafficops-go` image but not for `trafficops-perl` because the `trafficops-perl` service handled all of the database setup. Now that TO Perl is gone, we should remove `--nodeps` to assert that all of the necessary dependencies have been installed.
----------------------------------------------------------------
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] [trafficcontrol] TaylorCFrey commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
TaylorCFrey commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577979119
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -20,129 +20,76 @@
# Based on CentOS 8
############################################################
-# Keep the trafficops-common-deps in Dockerfile the same as
-# trafficops-common-deps in Dockerfile-go to cache the same
-# layer.
ARG RHEL_VERSION=8
-FROM centos:${RHEL_VERSION} as trafficops-common-deps
+FROM centos:${RHEL_VERSION}
ARG RHEL_VERSION=8
# Makes RHEL_VERSION available in later layers without needing to specify it again
ENV RHEL_VERSION=$RHEL_VERSION
RUN if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- yum -y install dnf || exit 1; \
- fi
+ yum -y install dnf || exit 1; \
+ fi
RUN set -o nounset -o errexit && \
- mkdir -p /etc/cron.d; \
- if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- use_repo=''; \
- else \
- use_repo='--repo=pgdg96'; \
- fi; \
- dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
- dnf -y $use_repo -- install postgresql96; \
- dnf -y install epel-release; \
- dnf -y install \
- jq \
- bind-utils \
- net-tools \
- gettext \
- perl-JSON-PP \
- mkisofs \
- isomd5sum \
- nmap-ncat \
- openssl \
- # Used to copy certs in "Shared SSL certificate generation" step
- rsync; \
- dnf clean all
-
-FROM trafficops-common-deps as trafficops-perl-deps
+ mkdir -p /etc/cron.d; \
+ if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
+ use_repo=''; \
+ else \
+ use_repo='--repo=pgdg96'; \
+ fi; \
+ dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
+ dnf -y $use_repo -- install postgresql96; \
+ dnf -y install epel-release; \
+ dnf -y install \
+ jq \
+ bind-utils \
+ net-tools \
+ gettext \
+ perl-JSON-PP \
+ perl-Crypt-ScryptKDF \
Review comment:
Do we still need these perl packages?
OIC, I think this is required because of the components like `adduser.pl` since it's requiring the _Crypt::ScriptKDF_ library.
Should we look into removing the perl scripts as a separate issue in the future?
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780728641
LGTM
Successfully tested api/v2/v3/v4 TO tests and TP/end-to-end on local CIAB with latest rpm and images.
Required Perl files have been removed and code modified correctly to ensure TO-Perl is no longer required.
PR can be merged once all the checks have successfully passed.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575458045
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
Don't we need to delette these .pl files too?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 edited a comment on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 edited a comment on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778501836
Perl module installs in the CiaB's "mid" Dockerfile are unrelated to Traffic Ops Perl, they're dependencies of the `traffic_ops_ort` RPM.
The `build-traffic_ops.log` and `build-traffic_ops_ort.log` files are not checked into the repository. They're generated by `pkg` when you build RPMs; they're literally just the logs from the Docker containers that build each component.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578018378
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
So, `postinstall` is unchanged. It even still calls `_postinstall` with the changes made in #4763. The postinstall.py is _added_ as an alternative only, but the default is to still use the Perl version.
Installation is still `yum install trops.rpm` and then run `postinstall`. configuration is mostly untouched, just some things don't have any purpose anymore. I guess the biggest change is that some scripts may expect that by setting their `PERL5LIB` path (or whatever it's called) that their dependencies will be satisfied, but that won't actually be true for new installations. A lot of those aren't used anymore afaik, and many others have active PRs rewriting them. Oh, osversion.cfg is gone, only osversions.json is supported now.
> Can you make a simlilar update to the ansible TO config templates?
I can try, but I don't know much about Ansible - I kind of assumed removing the Perl service might just break it all in half.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574728691
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/to-access.sh
##########
@@ -183,8 +182,7 @@ to-enroll() {
export MY_GATEWAY="$(route -n | grep $MY_NET_INTERFACE | grep -E '^0\.0\.0\.0' | tr -s ' ' | cut -d ' ' -f2)"
MY_NETMASK="$(ifconfig $MY_NET_INTERFACE | grep 'inet ' | tr -s ' ' | cut -d ' ' -f 5)"
export MY_NETMASK=${MY_NETMASK#"Mask:"}
- MY_IP6_ADDRESS="$(ifconfig $MY_NET_INTERFACE | grep inet6 | grep -i global | sed 's/addr://' | awk '{ print $2 }')"
- export MY_IP6_ADDRESS=${MY_IP6_ADDRESS%%/*}
Review comment:
Why was this removed?
----------------------------------------------------------------
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] [trafficcontrol] dewrich commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
dewrich commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780774047
👍 Nice work!
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578060157
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -73,7 +78,7 @@ ADD infrastructure/cdn-in-a-box/traffic_ops_data /traffic_ops_data
ARG TRAFFIC_OPS_RPM=infrastructure/cdn-in-a-box/traffic_ops/traffic_ops.rpm
COPY $TRAFFIC_OPS_RPM /traffic_ops.rpm
-RUN rpm --install --verbose --hash /traffic_ops.rpm && \
+RUN yum -y install /traffic_ops.rpm && \
Review comment:
interesting
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577940886
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -62,13 +62,13 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
+ context: ../../
+ dockerfile: traffic_ops/Dockerfile
Review comment:
Yep
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578018378
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
So, `postinstall` is unchanged. It even still calls `_postinstall` with the changes made in #4763. The postinstall.py is _added_ as an alternative only, but the default is to still use the Perl version.
Installation is still `yum install trops.rpm` and then run `postinstall`. configuration is mostly untouched, just some things don't have any purpose anymore. I guess the biggest change is that some scripts may expect that by setting their `PERL5LIB` path (or whatever it's called) that their dependencies will be satisfied, but that won't actually be true for new installations. A lot of those aren't used anymore afaik, and many others have active PRs rewriting them. Oh, osversion.cfg is gone, only osversions.json is supported now.
> Can you make a simlilar update to the ansible TO config templates?
I can try, but I don't know much about Ansible - I kind of assume removing the Perl service might just break it all in half.
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578058282
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -71,25 +62,12 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
Review comment:
I could have sworn this was working... maybe it was lost in the rebase.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574867210
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/to-access.sh
##########
@@ -183,8 +182,7 @@ to-enroll() {
export MY_GATEWAY="$(route -n | grep $MY_NET_INTERFACE | grep -E '^0\.0\.0\.0' | tr -s ' ' | cut -d ' ' -f2)"
MY_NETMASK="$(ifconfig $MY_NET_INTERFACE | grep 'inet ' | tr -s ' ' | cut -d ' ' -f 5)"
export MY_NETMASK=${MY_NETMASK#"Mask:"}
- MY_IP6_ADDRESS="$(ifconfig $MY_NET_INTERFACE | grep inet6 | grep -i global | sed 's/addr://' | awk '{ print $2 }')"
- export MY_IP6_ADDRESS=${MY_IP6_ADDRESS%%/*}
Review comment:
Well it wasn't removed, exactly, it was just changed from defining the variable on one line and exporting it on the next.
I'm not sure exactly what the second line was supposed to do (@zrhoffman would know) but it was crashing the script so I moved the export up, removed that line and now it works.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574733559
##########
File path: traffic_control/clients/python/pylint.rc
##########
@@ -259,7 +259,7 @@ redefining-builtins-modules=six.moves,past.builtins,future.builtins,builtins,io
# Format style used to check logging format string. `old` means using %
# formatting, while `new` is for `{}` formatting.
-logging-format-style=new
+logging-format-style=old
Review comment:
shouldn't we let it be new?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778460575
Yes, we do need the checks, still. People use those - for what I'm not sure, but they do run. They just make calls to the Traffic Ops API, so they should be unaffected if they're using supported API versions.
I do think we can get rid of `log4perl` configuration files under `traffic_ops/app/conf/` - so I'll do that - but I don't know what getting rid of that Jinja2 template will do to the Ansible stuff. Those playbooks are pretty much "experimental" in my estimation, so I'm happy to leave them alone for now.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575378015
##########
File path: infrastructure/cdn-in-a-box/traffic_vault/poststart.d/02-add-search-schema.sh
##########
@@ -16,7 +16,7 @@
# under the License.
source /to-access.sh
-curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml
+curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml || cat /var/log/riak/*
Review comment:
oh ok.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780750585
PR is good for merge.
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577871469
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -81,16 +72,6 @@ services:
args:
Review comment:
The context needs to be `../..` now
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -39,15 +39,6 @@ services:
- "2345:2345" #Delve debugging port
depends_on:
- trafficops-go-nondebug
Review comment:
The `-go` in `Dockerfile-go-debug`, `trafficops-go-debug`, and and `trafficops-go-nondebug` can be removed.
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -81,16 +72,6 @@ services:
args:
Review comment:
Same as above: The `-go` in `trafficops-go-nondebug`, `trafficops-go`, `trafficops-go-debug`, and `Dockerfile-go` can be removed.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578013231
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
I believe postinstall.pl is replaced by postinstall.py in this PR: https://github.com/apache/trafficcontrol/pull/4763
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577939478
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -62,13 +62,13 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
+ context: ../../
+ dockerfile: traffic_ops/Dockerfile
Review comment:
The RPM path in `infrastructure/cdn-in-a-box/traffic_ops/Dockerfile` is `infrastructure/cdn-in-a-box/traffic_ops/traffic_ops.rpm` - I think that's correct?
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r576927994
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -1826,32 +1804,18 @@ func DBStatsHandler(db *sqlx.DB) http.HandlerFunc {
}
}
-// RootHandler returns the / handler for the service, which reverse-proxies the old Perl Traffic Ops
-func rootHandler(d ServerData) http.Handler {
- tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
- DialContext: (&net.Dialer{
- Timeout: time.Duration(d.Config.ProxyTimeout) * time.Second,
- KeepAlive: time.Duration(d.Config.ProxyKeepAlive) * time.Second,
- }).DialContext,
- TLSHandshakeTimeout: time.Duration(d.Config.ProxyTLSTimeout) * time.Second,
- ResponseHeaderTimeout: time.Duration(d.Config.ProxyReadHeaderTimeout) * time.Second,
- //IdleConnTimeout: time.Duration(d.Config.ProxyIdleConnTimeout) * time.Second,
- //Other knobs we can turn: ExpectContinueTimeout,IdleConnTimeout
- }
- rp := httputil.NewSingleHostReverseProxy(d.URL)
- rp.Transport = tr
-
- rp.ErrorLog = log.StandardLogger(log.Error, "proxy error: ")
- riak.SetErrorLogger(log.StandardLogger(log.Error, "riak error: "))
- riak.SetLogger(log.StandardLogger(log.Info, "riak info: "))
+type root struct {
+ Handler http.Handler
+}
- log.Debugf("our reverseProxy: %++v\n", rp)
- log.Debugf("our reverseProxy's transport: %++v\n", tr)
- loggingProxyHandler := middleware.WrapAccessLog(d.Secrets[0], rp)
+func (root) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ alerts := tc.CreateAlerts(tc.ErrorLevel, fmt.Sprintf("The requested path '%s' does not exist.", r.URL.Path))
+ api.WriteAlerts(w, r, http.StatusNotFound, alerts)
+}
- managerHandler := CreateThrottledHandler(loggingProxyHandler, d.BackendMaxConnections["mojolicious"])
- return managerHandler
+// rootHandler returns the / handler for the service, which simply returns a "not found" response.
+func rootHandler(d ServerData) http.Handler {
+ return root{}
}
// notImplementedHandler returns a 501 Not Implemented to the client. This should be used very rarely, and primarily for old API Perl routes which were broken long ago, which we don't have the resources to rewrite in Go for the time being.
Review comment:
ok.. should we add a TODO? since v1API is on its way out soon..
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778440420
Also, the log files for perl? Should we delete them?
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575378752
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/to-access.sh
##########
@@ -183,8 +182,7 @@ to-enroll() {
export MY_GATEWAY="$(route -n | grep $MY_NET_INTERFACE | grep -E '^0\.0\.0\.0' | tr -s ' ' | cut -d ' ' -f2)"
MY_NETMASK="$(ifconfig $MY_NET_INTERFACE | grep 'inet ' | tr -s ' ' | cut -d ' ' -f 5)"
export MY_NETMASK=${MY_NETMASK#"Mask:"}
- MY_IP6_ADDRESS="$(ifconfig $MY_NET_INTERFACE | grep inet6 | grep -i global | sed 's/addr://' | awk '{ print $2 }')"
- export MY_IP6_ADDRESS=${MY_IP6_ADDRESS%%/*}
Review comment:
ok. Thanks for the explanation.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780701859
Also, are these two folders required?
What about this image?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578056501
##########
File path: .github/workflows/ciab.yaml
##########
@@ -229,11 +229,6 @@ jobs:
uses: actions/download-artifact@v2
with:
path: ${{ github.workspace }}/dist/
- - name: Cache Perl modules
- uses: actions/cache@v2
- with:
- path: ${{ github.workspace }}/infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-${{ env.RHEL_VERSION }}/local
Review comment:
I thought I got those already... done
----------------------------------------------------------------
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] [trafficcontrol] jhg03a commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
jhg03a commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578110623
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
Good point though on the PERL5Lib setup though if you'd like to remove it from https://github.com/apache/trafficcontrol/blob/5397a4fb17a24c255d698ce9594f37562b531c89/infrastructure/ansible/roles/traffic_ops/tasks/traffic_ops.yml#L127.
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780991971
Full command to reproduce that error:
```shell
RHEL_VERSION=7 docker-compose build trafficops
```
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577260669
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -1826,32 +1804,18 @@ func DBStatsHandler(db *sqlx.DB) http.HandlerFunc {
}
}
-// RootHandler returns the / handler for the service, which reverse-proxies the old Perl Traffic Ops
-func rootHandler(d ServerData) http.Handler {
- tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
- DialContext: (&net.Dialer{
- Timeout: time.Duration(d.Config.ProxyTimeout) * time.Second,
- KeepAlive: time.Duration(d.Config.ProxyKeepAlive) * time.Second,
- }).DialContext,
- TLSHandshakeTimeout: time.Duration(d.Config.ProxyTLSTimeout) * time.Second,
- ResponseHeaderTimeout: time.Duration(d.Config.ProxyReadHeaderTimeout) * time.Second,
- //IdleConnTimeout: time.Duration(d.Config.ProxyIdleConnTimeout) * time.Second,
- //Other knobs we can turn: ExpectContinueTimeout,IdleConnTimeout
- }
- rp := httputil.NewSingleHostReverseProxy(d.URL)
- rp.Transport = tr
-
- rp.ErrorLog = log.StandardLogger(log.Error, "proxy error: ")
- riak.SetErrorLogger(log.StandardLogger(log.Error, "riak error: "))
- riak.SetLogger(log.StandardLogger(log.Info, "riak info: "))
+type root struct {
+ Handler http.Handler
+}
- log.Debugf("our reverseProxy: %++v\n", rp)
- log.Debugf("our reverseProxy's transport: %++v\n", tr)
- loggingProxyHandler := middleware.WrapAccessLog(d.Secrets[0], rp)
+func (root) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ alerts := tc.CreateAlerts(tc.ErrorLevel, fmt.Sprintf("The requested path '%s' does not exist.", r.URL.Path))
+ api.WriteAlerts(w, r, http.StatusNotFound, alerts)
+}
- managerHandler := CreateThrottledHandler(loggingProxyHandler, d.BackendMaxConnections["mojolicious"])
- return managerHandler
+// rootHandler returns the / handler for the service, which simply returns a "not found" response.
+func rootHandler(d ServerData) http.Handler {
+ return root{}
}
// notImplementedHandler returns a 501 Not Implemented to the client. This should be used very rarely, and primarily for old API Perl routes which were broken long ago, which we don't have the resources to rewrite in Go for the time being.
Review comment:
Yeah, I can do that.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r576932358
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
I like the idea. Lets email and wait until Friday EOB.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780923501
> Does this mean we will also migrate traffic_ops_golang to just traffic_ops at some point?
I have a branch for that - I think it's called "making-go-a-first-class-citizen" or something - but it's woefully out-of-date.
Upshot is: I'm gonna try, at some point.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578016440
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -20,129 +20,76 @@
# Based on CentOS 8
############################################################
-# Keep the trafficops-common-deps in Dockerfile the same as
-# trafficops-common-deps in Dockerfile-go to cache the same
-# layer.
ARG RHEL_VERSION=8
-FROM centos:${RHEL_VERSION} as trafficops-common-deps
+FROM centos:${RHEL_VERSION}
ARG RHEL_VERSION=8
# Makes RHEL_VERSION available in later layers without needing to specify it again
ENV RHEL_VERSION=$RHEL_VERSION
RUN if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- yum -y install dnf || exit 1; \
- fi
+ yum -y install dnf || exit 1; \
+ fi
RUN set -o nounset -o errexit && \
- mkdir -p /etc/cron.d; \
- if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- use_repo=''; \
- else \
- use_repo='--repo=pgdg96'; \
- fi; \
- dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
- dnf -y $use_repo -- install postgresql96; \
- dnf -y install epel-release; \
- dnf -y install \
- jq \
- bind-utils \
- net-tools \
- gettext \
- perl-JSON-PP \
- mkisofs \
- isomd5sum \
- nmap-ncat \
- openssl \
- # Used to copy certs in "Shared SSL certificate generation" step
- rsync; \
- dnf clean all
-
-FROM trafficops-common-deps as trafficops-perl-deps
+ mkdir -p /etc/cron.d; \
+ if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
+ use_repo=''; \
+ else \
+ use_repo='--repo=pgdg96'; \
+ fi; \
+ dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
+ dnf -y $use_repo -- install postgresql96; \
+ dnf -y install epel-release; \
+ dnf -y install \
+ jq \
+ bind-utils \
+ net-tools \
+ gettext \
+ perl-JSON-PP \
+ perl-Crypt-ScryptKDF \
Review comment:
There are also a bunch of other scripts - like the checks - that have their own dependencies which were once part of the carton dependencies of the Perl version of Traffic Ops that we'll probably want to re-write (or remove, as appropriate) before too long.
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman merged pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman merged pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577948984
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -62,13 +62,13 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
+ context: ../../
+ dockerfile: traffic_ops/Dockerfile
Review comment:
Both `dockerfile` and `TRAFFIC_OPS_RPM` can have the same values as the `trafficops` service in `docker-compose.yml` has. It only needs to be specified again because the service name changes.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575540359
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -1826,32 +1804,18 @@ func DBStatsHandler(db *sqlx.DB) http.HandlerFunc {
}
}
-// RootHandler returns the / handler for the service, which reverse-proxies the old Perl Traffic Ops
-func rootHandler(d ServerData) http.Handler {
- tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
- DialContext: (&net.Dialer{
- Timeout: time.Duration(d.Config.ProxyTimeout) * time.Second,
- KeepAlive: time.Duration(d.Config.ProxyKeepAlive) * time.Second,
- }).DialContext,
- TLSHandshakeTimeout: time.Duration(d.Config.ProxyTLSTimeout) * time.Second,
- ResponseHeaderTimeout: time.Duration(d.Config.ProxyReadHeaderTimeout) * time.Second,
- //IdleConnTimeout: time.Duration(d.Config.ProxyIdleConnTimeout) * time.Second,
- //Other knobs we can turn: ExpectContinueTimeout,IdleConnTimeout
- }
- rp := httputil.NewSingleHostReverseProxy(d.URL)
- rp.Transport = tr
-
- rp.ErrorLog = log.StandardLogger(log.Error, "proxy error: ")
- riak.SetErrorLogger(log.StandardLogger(log.Error, "riak error: "))
- riak.SetLogger(log.StandardLogger(log.Info, "riak info: "))
+type root struct {
+ Handler http.Handler
+}
- log.Debugf("our reverseProxy: %++v\n", rp)
- log.Debugf("our reverseProxy's transport: %++v\n", tr)
- loggingProxyHandler := middleware.WrapAccessLog(d.Secrets[0], rp)
+func (root) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ alerts := tc.CreateAlerts(tc.ErrorLevel, fmt.Sprintf("The requested path '%s' does not exist.", r.URL.Path))
+ api.WriteAlerts(w, r, http.StatusNotFound, alerts)
+}
- managerHandler := CreateThrottledHandler(loggingProxyHandler, d.BackendMaxConnections["mojolicious"])
- return managerHandler
+// rootHandler returns the / handler for the service, which simply returns a "not found" response.
+func rootHandler(d ServerData) http.Handler {
+ return root{}
}
// notImplementedHandler returns a 501 Not Implemented to the client. This should be used very rarely, and primarily for old API Perl routes which were broken long ago, which we don't have the resources to rewrite in Go for the time being.
Review comment:
Is this function (notImplementedHandler) required? since it was primarily used for Perl routes?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778501836
Perl module installs in the CiaB's "mid" Dockerfile are unrelated to Traffic Ops Perl, they're dependencies of the `traffic_ops_ort` RPM.
The `build-traffic_ops.log` and build-traffic_ops_ort.log` files are not checked into the repository. They're generated by `pkg` when you build RPMs; they're literally just the logs from the Docker containers that build each component.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574733076
##########
File path: infrastructure/cdn-in-a-box/traffic_vault/poststart.d/02-add-search-schema.sh
##########
@@ -16,7 +16,7 @@
# under the License.
source /to-access.sh
-curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml
+curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml || cat /var/log/riak/*
Review comment:
I am not sure I understand why are we choosing data set as ssl keys or riak log? shouldn't it be only ssl keys unless riak logs = ssl keys
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577916359
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -32,13 +32,13 @@ services:
- trafficmonitor-nondebug
trafficops:
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go-debug
- image: trafficops-go-debug
+ context: ../..
Review comment:
Context for this one can stay `.`. Otherwise, it won't find the Dockerfile
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -62,13 +62,13 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
+ context: ../../
+ dockerfile: traffic_ops/Dockerfile
Review comment:
Building `trafficops-nondebug` fails:
```dockerfile
Building trafficops-nondebug
unable to prepare context: unable to evaluate symlinks in Dockerfile path: lstat /home/user/go/src/github.com/apache/trafficcontrol/traffic_ops/Dockerfile: no such file or directory
Service 'trafficops-nondebug' failed to build
```
You'll need to change the rpm path, too.
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578052559
##########
File path: .github/workflows/ciab.yaml
##########
@@ -229,11 +229,6 @@ jobs:
uses: actions/download-artifact@v2
with:
path: ${{ github.workspace }}/dist/
- - name: Cache Perl modules
- uses: actions/cache@v2
- with:
- path: ${{ github.workspace }}/infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-${{ env.RHEL_VERSION }}/local
Review comment:
Nit: The `infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-7/local/.gitkeep` and `infrastructure/cdn-in-a-box/traffic_ops/perl-cache/centos-7/local/.gitkeep` files can also be removed.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574868699
##########
File path: infrastructure/cdn-in-a-box/traffic_vault/poststart.d/02-add-search-schema.sh
##########
@@ -16,7 +16,7 @@
# under the License.
source /to-access.sh
-curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml
+curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml || cat /var/log/riak/*
Review comment:
The or here is not being applied to what data to use, it's the second-lowest shell operator binding. So this isn't
```bash
curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d (@/sslkeys.xml || cat /var/log/riak/*)
```
(which I don't think would even actually work like that), it's
```bash
(curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml) || (cat /var/log/riak/*)
```
that is, the second command runs if the first one fails.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r576378718
##########
File path: traffic_ops/traffic_ops_golang/routing/routes.go
##########
@@ -1826,32 +1804,18 @@ func DBStatsHandler(db *sqlx.DB) http.HandlerFunc {
}
}
-// RootHandler returns the / handler for the service, which reverse-proxies the old Perl Traffic Ops
-func rootHandler(d ServerData) http.Handler {
- tr := &http.Transport{
- TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
- DialContext: (&net.Dialer{
- Timeout: time.Duration(d.Config.ProxyTimeout) * time.Second,
- KeepAlive: time.Duration(d.Config.ProxyKeepAlive) * time.Second,
- }).DialContext,
- TLSHandshakeTimeout: time.Duration(d.Config.ProxyTLSTimeout) * time.Second,
- ResponseHeaderTimeout: time.Duration(d.Config.ProxyReadHeaderTimeout) * time.Second,
- //IdleConnTimeout: time.Duration(d.Config.ProxyIdleConnTimeout) * time.Second,
- //Other knobs we can turn: ExpectContinueTimeout,IdleConnTimeout
- }
- rp := httputil.NewSingleHostReverseProxy(d.URL)
- rp.Transport = tr
-
- rp.ErrorLog = log.StandardLogger(log.Error, "proxy error: ")
- riak.SetErrorLogger(log.StandardLogger(log.Error, "riak error: "))
- riak.SetLogger(log.StandardLogger(log.Info, "riak info: "))
+type root struct {
+ Handler http.Handler
+}
- log.Debugf("our reverseProxy: %++v\n", rp)
- log.Debugf("our reverseProxy's transport: %++v\n", tr)
- loggingProxyHandler := middleware.WrapAccessLog(d.Secrets[0], rp)
+func (root) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+ alerts := tc.CreateAlerts(tc.ErrorLevel, fmt.Sprintf("The requested path '%s' does not exist.", r.URL.Path))
+ api.WriteAlerts(w, r, http.StatusNotFound, alerts)
+}
- managerHandler := CreateThrottledHandler(loggingProxyHandler, d.BackendMaxConnections["mojolicious"])
- return managerHandler
+// rootHandler returns the / handler for the service, which simply returns a "not found" response.
+func rootHandler(d ServerData) http.Handler {
+ return root{}
}
// notImplementedHandler returns a 501 Not Implemented to the client. This should be used very rarely, and primarily for old API Perl routes which were broken long ago, which we don't have the resources to rewrite in Go for the time being.
Review comment:
Yeah, that's going to need to stick around as long as APIv1 exists. It's basically used to fill "holes" that the Go implementation has in the v1 API.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575458045
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
Don't we need to delete these .pl files too? as in not only from this file but actually delete them from script folder, like you did with cdn file?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r574872972
##########
File path: traffic_control/clients/python/pylint.rc
##########
@@ -259,7 +259,7 @@ redefining-builtins-modules=six.moves,past.builtins,future.builtins,builtins,io
# Format style used to check logging format string. `old` means using %
# formatting, while `new` is for `{}` formatting.
-logging-format-style=new
+logging-format-style=old
Review comment:
The new logging format isn't actually supported, Python the project has said they're just going to continue using the old format to avoid breaking people. I don't remember specifics, but I do remember being extremely annoyed by an inability to use the "new" format.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578126406
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
I think it's fine to leave that there. If it's an existing install that's where the libraries will already be, and if it's a new install I don't think that'll hurt anything.
----------------------------------------------------------------
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] [trafficcontrol] dneuman64 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
dneuman64 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-780786114
RIP Perl
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778498389
Since perl is removed, we need to removed it from README too
Also, build-traffic_ops.logs and build-traffic_ops_ort.log should remove installation steps for perl
ciab-rst needs a cleanup too
traffic_ops.rst under admin and development folder need cleanup wrt traffic_ops-perl reference
need to remove perl module installs from ciab/mid/Dockerfile
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575378413
##########
File path: traffic_control/clients/python/pylint.rc
##########
@@ -259,7 +259,7 @@ redefining-builtins-modules=six.moves,past.builtins,future.builtins,builtins,io
# Format style used to check logging format string. `old` means using %
# formatting, while `new` is for `{}` formatting.
-logging-format-style=new
+logging-format-style=old
Review comment:
copy!
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578054744
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -73,7 +78,7 @@ ADD infrastructure/cdn-in-a-box/traffic_ops_data /traffic_ops_data
ARG TRAFFIC_OPS_RPM=infrastructure/cdn-in-a-box/traffic_ops/traffic_ops.rpm
COPY $TRAFFIC_OPS_RPM /traffic_ops.rpm
-RUN rpm --install --verbose --hash /traffic_ops.rpm && \
+RUN yum -y install /traffic_ops.rpm && \
Review comment:
On CentOS 8, `yum` is just a symlink to `dnf`
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575458045
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
Don't we need to delette these .pl files too? as in not only from this file but actually delete them from script folder, like you did with cdn file?
----------------------------------------------------------------
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] [trafficcontrol] dewrich commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
dewrich commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-781462915
Nice work!
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r577995677
##########
File path: infrastructure/cdn-in-a-box/traffic_ops/Dockerfile
##########
@@ -20,129 +20,76 @@
# Based on CentOS 8
############################################################
-# Keep the trafficops-common-deps in Dockerfile the same as
-# trafficops-common-deps in Dockerfile-go to cache the same
-# layer.
ARG RHEL_VERSION=8
-FROM centos:${RHEL_VERSION} as trafficops-common-deps
+FROM centos:${RHEL_VERSION}
ARG RHEL_VERSION=8
# Makes RHEL_VERSION available in later layers without needing to specify it again
ENV RHEL_VERSION=$RHEL_VERSION
RUN if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- yum -y install dnf || exit 1; \
- fi
+ yum -y install dnf || exit 1; \
+ fi
RUN set -o nounset -o errexit && \
- mkdir -p /etc/cron.d; \
- if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
- use_repo=''; \
- else \
- use_repo='--repo=pgdg96'; \
- fi; \
- dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
- dnf -y $use_repo -- install postgresql96; \
- dnf -y install epel-release; \
- dnf -y install \
- jq \
- bind-utils \
- net-tools \
- gettext \
- perl-JSON-PP \
- mkisofs \
- isomd5sum \
- nmap-ncat \
- openssl \
- # Used to copy certs in "Shared SSL certificate generation" step
- rsync; \
- dnf clean all
-
-FROM trafficops-common-deps as trafficops-perl-deps
+ mkdir -p /etc/cron.d; \
+ if [[ "${RHEL_VERSION%%.*}" -eq 7 ]]; then \
+ use_repo=''; \
+ else \
+ use_repo='--repo=pgdg96'; \
+ fi; \
+ dnf -y install "https://download.postgresql.org/pub/repos/yum/reporpms/EL-${RHEL_VERSION%%.*}-x86_64/pgdg-redhat-repo-latest.noarch.rpm"; \
+ dnf -y $use_repo -- install postgresql96; \
+ dnf -y install epel-release; \
+ dnf -y install \
+ jq \
+ bind-utils \
+ net-tools \
+ gettext \
+ perl-JSON-PP \
+ perl-Crypt-ScryptKDF \
Review comment:
> Should we look into removing the perl scripts as a separate issue in the future?
For sure. All of the Perl cryptography functions it uses are reimplemented in Go and used by traffic_ops_golang, so we should have everything we need to rewrite it.
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#issuecomment-778440108
Do we need these files in the repo?
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575474674
##########
File path: traffic_ops/build/traffic_ops.spec
##########
@@ -206,23 +203,16 @@ if [ "$1" = "0" ]; then
# this is an uninstall
%__rm -rf %{PACKAGEDIR}
%__rm /etc/init.d/traffic_ops
- /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
- /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
+ /usr/bin/getent passwd %{TRAFFIC_OPS_USER} || /usr/sbin/userdel %{TRAFFIC_OPS_USER}
+ /usr/bin/getent group %{TRAFFIC_OPS_GROUP} || /usr/sbin/groupdel %{TRAFFIC_OPS_GROUP}
fi
%files
%license ../LICENSE
%defattr(644,root,root,755)
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/config2json
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/extensions
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/osversions-convert.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/routes.pl
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/start.pl
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/bin/traffic_ops_golang
-%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/cdn
%attr(755,%{TRAFFIC_OPS_USER},%{TRAFFIC_OPS_GROUP}) %{PACKAGEDIR}/app/script/detect10ginterfaces.pl
Review comment:
I don't think we should, those scripts might still be in use. The `cdn` script was deleted because it was the entrypoint for Traffic Ops Perl - or one of them, anyway. I think there were like 3 ways to start it.
----------------------------------------------------------------
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] [trafficcontrol] jhg03a commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
jhg03a commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578109852
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
You're right, it looks like it was added and will be removed before anyone uses it in https://github.com/jhg03a/trafficcontrol/blob/ansible.refactor/infrastructure/ansible/roles/traffic_ops/templates/cdn.conf.j2
----------------------------------------------------------------
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] [trafficcontrol] jhg03a commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
jhg03a commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578005108
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
Can you make a simlilar update to the ansible TO config templates?
----------------------------------------------------------------
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] [trafficcontrol] rimashah25 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
rimashah25 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r575378015
##########
File path: infrastructure/cdn-in-a-box/traffic_vault/poststart.d/02-add-search-schema.sh
##########
@@ -16,7 +16,7 @@
# under the License.
source /to-access.sh
-curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml
+curl -kvs -XPUT -H 'Content-Type:application/xml' "https://$TV_ADMIN_USER:$TV_ADMIN_PASSWORD@$TV_FQDN:$TV_HTTPS_PORT/search/schema/sslkeys" -d @/sslkeys.xml || cat /var/log/riak/*
Review comment:
oh ok. got it.
----------------------------------------------------------------
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] [trafficcontrol] ocket8888 commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
ocket8888 commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578047655
##########
File path: traffic_ops/app/conf/cdn.conf
##########
@@ -36,7 +36,6 @@
"oauth_client_secret": "",
"routing_blacklist": {
"ignore_unknown_routes": false,
- "perl_routes": [],
Review comment:
So, I can't find any reference to this configuration file option in the Ansible files:
```shell-session
$ grep -r 'perl_routes' ./infrastructure/ansible | wc -l
0
```
----------------------------------------------------------------
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] [trafficcontrol] zrhoffman commented on a change in pull request #5209: Perlectomy
Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #5209:
URL: https://github.com/apache/trafficcontrol/pull/5209#discussion_r578056693
##########
File path: infrastructure/cdn-in-a-box/optional/docker-compose.debugging.yml
##########
@@ -71,25 +62,12 @@ services:
args:
TRAFFIC_MONITOR_RPM: traffic_monitor/traffic_monitor.rpm
command: /usr/bin/true
- # The trafficops-go-nondebug service ensures that the trafficops-go base image
- # exists before trying to build trafficops-go-debug.
- trafficops-go-nondebug:
- image: trafficops-go
+ # The trafficops-nondebug service ensures that the trafficops base image
+ # exists before trying to build trafficops-debug.
+ trafficops-nondebug:
+ image: trafficops
build:
- context: .
- dockerfile: traffic_ops/Dockerfile-go
Review comment:
It's still not finding the Dockerfile because the Dockerfile doesn't exist in the directory of the build context. Its path needs to be specified.
```
[user@computer cdn-in-a-box]$ docker-compose -f docker-compose.yml -f optional/docker-compose.debugging.yml build trafficops-nondebug
Building with native build. Learn about native build in Compose here: https://docs.docker.com/go/compose-native-build/
Building trafficops-nondebug
unable to prepare context: unable to evaluate symlinks in Dockerfile path: lstat /home/user/go/src/github.com/apache/trafficcontrol/Dockerfile: no such file or directory
```
----------------------------------------------------------------
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