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.

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

Remove operator-sdk new|add api commands and ansible scaffolding. #3531

Conversation

varshaprasad96
Copy link
Member

@varshaprasad96 varshaprasad96 commented Jul 24, 2020

Description of the change:
Remove operator-sdk new|add api commands and ansible scaffolding.
This commit does the following:

  1. Remove operator-sdk new and operator-sdk add api in favour of ansible plugins.
  2. Remove ansible scaffolding (ie) internal/scaffold/ansible.
  3. Remove internal/flags/apiflags/flags.go and relevant tests which were used in the commands.
  4. Update IsOperatorAnsible() to check only for new layouts.
  5. Remove reference to new|add api commands in cli.

Motivation for the change:
Remove support for legacy commands and layouts in Operator SDK 1.0

Things to-do in follow up PR:

  1. Remove operator-sdk add crd and relevant crd generator code/its tests which are not used.
  2. Remove legacy cli support from cmd/operator-sdk/main.go

c/c @joelanford @jmrodri @camilamacedo86 @fabianvf

Summary of changes:

A       changelog/fragments/rm-cmd-legacy-ansible.yaml
D       cmd/operator-sdk/add/api.go
M       cmd/operator-sdk/add/cmd.go
M       cmd/operator-sdk/cli/cli.go
M       cmd/operator-sdk/cli/legacy.go
M       cmd/operator-sdk/main.go
D       cmd/operator-sdk/new/cmd.go
M       hack/tests/e2e-ansible.sh
D       internal/flags/apiflags/flags.go
D       internal/flags/apiflags/flags_test.go
D       internal/scaffold/ansible/OWNERS
D       internal/scaffold/ansible/build_dockerfile.go
D       internal/scaffold/ansible/constants.go
D       internal/scaffold/ansible/deploy_operator.go
D       internal/scaffold/ansible/input.go
D       internal/scaffold/ansible/molecule_cluster_converge.go
D       internal/scaffold/ansible/molecule_cluster_create.go
D       internal/scaffold/ansible/molecule_cluster_destroy.go
D       internal/scaffold/ansible/molecule_cluster_molecule.go
D       internal/scaffold/ansible/molecule_cluster_prepare.go
D       internal/scaffold/ansible/molecule_cluster_verify.go
D       internal/scaffold/ansible/molecule_default_converge.go
D       internal/scaffold/ansible/molecule_default_molecule.go
D       internal/scaffold/ansible/molecule_default_prepare.go
D       internal/scaffold/ansible/molecule_default_verify.go
D       internal/scaffold/ansible/molecule_templates_operator.go
D       internal/scaffold/ansible/molecule_test_local_converge.go
D       internal/scaffold/ansible/molecule_test_local_molecule.go
D       internal/scaffold/ansible/molecule_test_local_prepare.go
D       internal/scaffold/ansible/molecule_test_local_verify.go
D       internal/scaffold/ansible/playbook.go
D       internal/scaffold/ansible/requirements.go
D       internal/scaffold/ansible/roles_defaults_main.go
D       internal/scaffold/ansible/roles_files.go
D       internal/scaffold/ansible/roles_handlers_main.go
D       internal/scaffold/ansible/roles_meta_main.go
D       internal/scaffold/ansible/roles_readme.go
D       internal/scaffold/ansible/roles_tasks_main.go
D       internal/scaffold/ansible/roles_templates.go
D       internal/scaffold/ansible/roles_vars_main.go
D       internal/scaffold/ansible/testdata/invalid/emptywatchfile/watches.yaml
D       internal/scaffold/ansible/testdata/invalid/invalid_watch/watches.yaml
D       internal/scaffold/ansible/testdata/valid1/validWatches.yaml
D       internal/scaffold/ansible/testdata/valid2/validWatches.yaml
D       internal/scaffold/ansible/travis.go
D       internal/scaffold/ansible/watches.go
D       internal/scaffold/ansible/watches_test.go
M       internal/util/projutil/project_util.go
M       website/content/en/docs/cli/operator-sdk.md
D       website/content/en/docs/cli/operator-sdk_new.md

Checklist

If the pull request includes user-facing changes, extra documentation is required:

.travis.yml Outdated
@@ -117,6 +117,7 @@ jobs:
## Operator test stage jobs ##

# Build and test ansible and test ansible using molecule
# TODO: Replace with the required scripts after ansible plugin PR is merged
Copy link
Member Author

Choose a reason for hiding this comment

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

Leaving install set up here, thought not necessary.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 24, 2020

popd
popd
# TODO: Include test script after ansible plugins PR is merged.
Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the full file and target.

Copy link
Contributor

Choose a reason for hiding this comment

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

but do not need to do it in this PR we can do in a follow-up. Let's save Travis cycles.

.travis.yml Outdated
@@ -125,7 +126,6 @@ jobs:
- pip install --user ansible
script:
- make test-e2e-ansible
- make test-e2e-ansible-molecule

# Test subcommands
- <<: *test
Copy link
Contributor

Choose a reason for hiding this comment

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

Shows that we can remove all: /operator-sdk/internal/scaffold/
Required check if has some part of code which will require it.

Copy link
Member Author

@varshaprasad96 varshaprasad96 Jul 24, 2020

Choose a reason for hiding this comment

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

Some of it is still used while scaffolding kustomize templates (eg: in generate/kustomize/manifests.go) and in add crd.
With the removal of add crd, Ill clean up internal/scaffold too.

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

You can remove it if pass in the CI and then we can do follow-ups with what can be missing here👍

@@ -1,69 +0,0 @@
#!/usr/bin/env bash
Copy link
Member

Choose a reason for hiding this comment

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

@fabianvf since you are modifying this file for the ansible-plugin, I assume @varshaprasad96 should leave this file untouched?

Copy link
Member

Choose a reason for hiding this comment

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

correct

Copy link
Member

Choose a reason for hiding this comment

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

they've been rewritten in the scaffold PR, this will just cause a conflict

Copy link
Member Author

Choose a reason for hiding this comment

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

Ill add this, and comment the targets in Makefile where its called.

Copy link
Member

Choose a reason for hiding this comment

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

e2e tests will fail if the scaffolding is removed. so we either need to block this PR on the plugin PR

Or have the plugin PR rebase and re-add these tests.

Either way is fine by me.

@joelanford joelanford mentioned this pull request Jul 24, 2020
92 tasks
@varshaprasad96
Copy link
Member Author

/hold Will make changes after #3488 gets merged.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 24, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2020
@varshaprasad96 varshaprasad96 removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2020
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

Just one question

hack/tests/e2e-ansible.sh Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2020
This commit does the following:
1. Remove operator-sdk new  and operator-sdk add api in favour of ansible plugins.
2. Remove ansible scaffolding (ie) internal/scaffold/ansible.
3. Remove internal/flags/apiflags/flags.go  and relevant tests which were used in the commands.
4. Update IsOperatorAnsible() to check only for new layouts.
5. Remove hack/tests/e2e-ansible-molecule.sh and rewrite hack/tests/e2e-ansible.sh (which will get updated after ansible plugins).
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2020
@joelanford joelanford merged commit 47392df into operator-framework:master Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants