Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

rollback getTechnical in SSH #353

Closed

Conversation

tedteng
Copy link
Contributor

@tedteng tedteng commented Sep 28, 2020

What this PR does / why we need it:
rollback change getTechnial in SSH

Which issue(s) this PR fixes:
Fixes #322

Special notes for your reviewer:

Release note:


@tedteng tedteng requested a review from a team as a code owner September 28, 2020 07:24
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 28, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Sep 28, 2020
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 28, 2020
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Sep 28, 2020
Copy link
Contributor

@petersutter petersutter left a comment

Choose a reason for hiding this comment

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

This is not an acceptable solution. It does not fix the root cause and in addition it re-introduces old code that was replaced with the new getTechnicalID implementation by a reason

  • fetch all shoots across all namespaces (which is not necessary)
  • fragile if strings.HasSuffix(namespace.Name, clustername) { check

@tedteng
Copy link
Contributor Author

tedteng commented Sep 29, 2020

This is not an acceptable solution. It does not fix the root cause and in addition it re-introduces old code that was replaced with the new getTechnicalID implementation by a reason

  • fetch all shoots across all namespaces (which is not necessary)
  • fragile if strings.HasSuffix(namespace.Name, clustername) { check

This PR only rollback change getTechnicalID to original. which block the neo testing his own code in his local dev environment .I believe this will change it again when getFromTargetInfo done.

To fix the root cause It will be in the ticket issue #354

Copy link
Contributor

@neo-liang-sap neo-liang-sap left a comment

Choose a reason for hiding this comment

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

I agree with @petersutter , this is not acceptable.
@tedteng , please take your time to work on #322 and fix it from root cause, as i mentioned in #344 (review) , we don't have tight release schedule here.
BTW i don't have any urgent testing blocked here, so again, take your time.

@tedteng
Copy link
Contributor Author

tedteng commented Oct 8, 2020

/close it as an issue in DEV local environment and not critical any more

@tedteng tedteng deleted the remove_getTechnicalfrom_ssh branch November 25, 2020 03:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
6 participants