Removes dead code for conflict-test #1797
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schnie
commented
Jan 30, 2025
| @@ -30,7 +30,6 @@ import ( | |||
| ) | |||
|
|
|||
| const ( | |||
| EchoCmd = "echo" | |||
There was a problem hiding this comment.
Unrelated, but also unused.
schnie
commented
Jan 30, 2025
| @@ -60,7 +60,6 @@ var ( | |||
| envExport bool | |||
| noBrowser bool | |||
| compose bool | |||
| conflictTest bool | |||
There was a problem hiding this comment.
I don't see anywhere in our code that sets this variable. It used to exist at one time as a flag, but doesn't anymore. This would always evaluate to false, so we remove the code that branches if this value is true.
schnie
commented
Jan 30, 2025
Comment on lines
-602
to
-607
| if conflictTest { | ||
| err = d.conflictTest(testHomeDirectory, newImageName, newAirflowVersion) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
Nowhere in our code base do we seem to set conflictTest to true. There should be no way for this branch to ever run. This conflictTest used to exist as a flag on the CLI commands, but seems to have been removed.
schnie
force-pushed
the
remove-conflict-test-dead-code
branch
from
b1ca490 to
e0230f8
Compare
schnie
requested review from
neel-astro,
jeremybeard,
kushalmalani and
lzdanski
as code owners
…r/astro-cli into remove-conflict-test-dead-code
kushalmalani
approved these changes
Jan 31, 2025
neel-astro
approved these changes
Feb 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR removes some seemingly dead code. This
conflictTestvariable will always evaluate tofalse, so we're removing all the code that would run if we branched on this value beingtrue. Seems like it used to be a flag that we've removed along the way. Not sure how else this code would ever run.Update: I found this digging through the history. Looks like we intentionally removed the functionality, but didn't remove the dead code. @kushalmalani even suggested it here.
🎟 Issue(s)
No issue, just noticed while looking at other things.
🧪 Functional Testing
📸 Screenshots
📋 Checklist
make testbefore taking out of draftmake lintbefore taking out of draft