Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ENH] - Include "--attempt-fixes" flag from Nebari upgrade CLI in upgrade steps logic #2839

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

[ENH] - Include "--attempt-fixes" flag from Nebari upgrade CLI in upgrade steps logic #2839

wants to merge 49 commits into from
+401 −86

Conversation

smokestacklightnin
Copy link

@smokestacklightnin smokestacklightnin commented Nov 10, 2024 •
edited by viniciusdc
Loading

Reference Issues or PRs

This resolves #2761 by passing the attempt_fixes variable from the command line option to the upgrade methods.
close #2761

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

How to test this PR?

Run nebari upgrade --attempt-fixes and verify that the upgrade is successful without prompts from the user.

Any other comments?

@smokestacklightnin smokestacklightnin force-pushed the commands/upgrade/attempt-fix-logic branch 4 times, most recently from 2b5260e to d053d04 Compare November 11, 2024 01:18
@smokestacklightnin smokestacklightnin marked this pull request as ready for review November 11, 2024 01:43
@dcmcand
Copy link
Contributor

dcmcand commented Nov 11, 2024

This should probably be a draft PR if you still have questions about the implementation

@dcmcand dcmcand marked this pull request as draft November 11, 2024 08:59
@smokestacklightnin smokestacklightnin force-pushed the commands/upgrade/attempt-fix-logic branch from d053d04 to 47df80c Compare November 15, 2024 03:59
marcelovilla
marcelovilla requested changes Nov 15, 2024
View reviewed changes
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
src/_nebari/upgrade.py Outdated Show resolved Hide resolved
@marcelovilla
Copy link
Member

@smokestacklightnin can you also make sure the tests are passing?

@smokestacklightnin
Autoformat from Black
28db7be
@smokestacklightnin
Add attempt_fixes parameter to UpgradeStep.upgrade_step and `Upgr…
f387e4b 
…adeStep._version_specific_upgrade`
@smokestacklightnin
Add attempt_fixes to UpgradeStep.replace_image_tag and `UpgradeSt…
06598c7 
…ep.update_image_tag`
@smokestacklightnin
Add attempt_fixes to Upgrade_2023_4_2._version_specific_upgrade
3bf0d4d
@smokestacklightnin
Add attempt_fixes to Upgrade_2023_7_2._version_specific_upgrade
a312463
@smokestacklightnin
Add attempt_fixes to Upgrade_2024_4_1._version_specific_upgrade
0359281
@smokestacklightnin
Add attempt_fixes to Upgrade_2024_6_1._version_specific_upgrade
a1eefe2
@smokestacklightnin
Add attempt_fixes to Upgrade_2024_11_1._version_specific_upgrade
2ee366c
@smokestacklightnin
Remove unnecessary argument
a6a0364
@smokestacklightnin
Remove unnecessary "[y/N]"
804edfd
@smokestacklightnin
Use attempt_fixes from kwargs
8f8215a
@smokestacklightnin
Remove unnecessary default value
6fc2dea
@smokestacklightnin
Remove unnecessary inclusion of attempt_fixes
48a5ae4
@smokestacklightnin
Refactor Argo delete commands
38e5a48
@smokestacklightnin
Fix quotation marks
239c419
@smokestacklightnin smokestacklightnin force-pushed the commands/upgrade/attempt-fix-logic branch from 4807612 to 239c419 Compare November 20, 2024 07:13
@smokestacklightnin smokestacklightnin force-pushed the commands/upgrade/attempt-fix-logic branch from adade08 to d7520ed Compare November 21, 2024 07:39
@smokestacklightnin smokestacklightnin marked this pull request as ready for review December 19, 2024 20:15
@smokestacklightnin
Copy link
Author

smokestacklightnin commented Dec 19, 2024 •
edited
Loading

@marcelovilla All tests are passing. There is only one test that fails because it needs monkeypatching (CC: @viniciusdc). I have xfailed it for now.

@smokestacklightnin smokestacklightnin marked this pull request as draft December 23, 2024 19:53
@marcelovilla
Copy link
Member

All tests are passing. There is only one test that fails because it needs monkeypatching (CC: @viniciusdc). I have xfailed it for now.

@smokestacklightnin I think that should have been addressed by #2879. Or am I confusing tests here?

marcelovilla
marcelovilla reviewed Dec 26, 2024
View reviewed changes
src/_nebari/upgrade.py
default="N",
).lower()
== "y"
# This needs to be monkeypatched and will be addressed in a future PR. Until then, this causes test failures.
Copy link
Member

Choose a reason for hiding this comment

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

I believe this has been solved in https://github.com/nebari-dev/nebari/pull/2879/files

marcelovilla
marcelovilla previously approved these changes Dec 26, 2024
View reviewed changes
@marcelovilla marcelovilla self-requested a review December 26, 2024 16:25
@marcelovilla marcelovilla dismissed their stale review December 26, 2024 16:26

I need to test the latest changes

marcelovilla
marcelovilla reviewed Dec 26, 2024
View reviewed changes
Copy link
Member

@marcelovilla marcelovilla left a comment

Choose a reason for hiding this comment

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

@smokestacklightnin I just tested an upgrade and it seems the flag is working as expected 🚀

The only thing I want to sort out before approving the PR is the test you xfailed for now. Is that still needed?

@smokestacklightnin smokestacklightnin marked this pull request as ready for review January 3, 2025 08:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcmcand dcmcand dcmcand left review comments

@viniciusdc viniciusdc Awaiting requested review from viniciusdc

@marcelovilla marcelovilla Awaiting requested review from marcelovilla

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
🪴 Nebari Project Management
Status: Changes requested 🧱
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ENH] - Include "--attempt-fixes" flag from Nebari upgrade CLI in upgrade steps logic
4 participants
@smokestacklightnin @dcmcand @marcelovilla @viniciusdc