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

Unify Ankaios cli #340

Merged
merged 18 commits into from
Aug 21, 2024
Merged

Unify Ankaios cli #340

merged 18 commits into from
Aug 21, 2024

Conversation

HorjuRares
Copy link
Contributor

@HorjuRares HorjuRares commented Aug 6, 2024 •
edited by inf17101
Loading

Issue-Id: #173

@inf17101 inf17101 changed the title 173 unify ankaios cli Unify Ankaios cli Aug 12, 2024
@inf17101
Copy link
Contributor

@HorjuRares: It seems like 7 system tests are failing. This needs to be fixed.

inf17101
inf17101 reviewed Aug 12, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Review finished.

@HorjuRares: The Ankaios cli is now unifed for the user. But I see internally that the check if we use a stdin reader or reading from file is still duplicated. It is used inside the ank apply and set state functionality. I am thinking about how this can be done and we will open an issue if it is more effort and not doing it here inside this PR.

ank/src/cli_commands/set_state.rs Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli.rs Outdated Show resolved Hide resolved
@inf17101 inf17101 mentioned this pull request Aug 12, 2024
Closed
4 tasks
inf17101
inf17101 reviewed Aug 15, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

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

Review done. Some small refactoring suggestions also from a method that is not directly touched inside the PR, but it is good because we anyway work on this code path.

ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
inf17101
inf17101 reviewed Aug 19, 2024
View reviewed changes
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Outdated Show resolved Hide resolved
ank/src/cli_commands/set_state.rs Show resolved Hide resolved
@inf17101
Copy link
Contributor

inf17101 commented Aug 19, 2024 •
edited
Loading

I think if the last findings will be resolved, then we can merge the PR.
@HorjuRares: Could you create an enhancement issue about refactoring the two occurrences of the match checking for stdin and file name input to a single place/function?

@HorjuRares @inf17101
Link swdd
d2f4039 
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
@inf17101 inf17101 added enhancement New feature or request. Issue will appear in the change log "Features" and removed ready for review labels Aug 20, 2024
@inf17101
Copy link
Contributor

inf17101 commented Aug 20, 2024 •
edited
Loading

@HorjuRares: The user documentation must be updated, since the set state command has been changed. Could you fix that? I will check the swdd requirements because some might be need a change.

inf17101
inf17101 approved these changes Aug 20, 2024
View reviewed changes
Copy link
Contributor

@inf17101 inf17101 left a comment

Choose a reason for hiding this comment

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

LGTM👍

@inf17101
Copy link
Contributor

inf17101 commented Aug 20, 2024 •
edited
Loading

I tested the new version of the command to pipe in the changed CompleteState with the nginx restartPolicy of the Working with the CompleteState section and it works. So, I think we can merge it when the last finding is resolved.

This is how the new version looks when using stdin:

cat new-state.yaml | ./ank -k set state desiredState.workloads.nginx.restartPolicy -

@HorjuRares @inf17101
Update unit test
0fbd89f 
Co-authored-by: Oliver <42932060+inf17101@users.noreply.github.com>
@inf17101 inf17101 merged commit fed2801 into eclipse-ankaios:main Aug 21, 2024
10 checks passed
@inf17101 inf17101 added breaking-change Issue will appear in the change log "Breaking Changes" and removed ready for review enhancement New feature or request. Issue will appear in the change log "Features" labels Aug 21, 2024
@inf17101 inf17101 mentioned this pull request Oct 21, 2024
Closed
@HorjuRares HorjuRares deleted the 173_unify_ankaios_cli branch November 11, 2024 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inf17101 inf17101 inf17101 approved these changes

Assignees

@HorjuRares HorjuRares

Labels
breaking-change Issue will appear in the change log "Breaking Changes"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@HorjuRares @inf17101