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

Deprecated test.TestCtx in order to rename test.TestCtx to test.Context #2549

Merged
merged 7 commits into from
Feb 27, 2020
Merged

Deprecated test.TestCtx in order to rename test.TestCtx to test.Context #2549

merged 7 commits into from
Feb 27, 2020

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Feb 11, 2020 •
edited
Loading

Description of the change:

  • Deprecated test.TestCtx in order to rename test.TestCtx to test.Context

Motivation for the change:

  • Following the good practices since we should not have test.Test;
  • Be able to rename the API before 1.0.0
  • Solve tech-debts

@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 11, 2020
estroz
estroz suggested changes Feb 11, 2020
View reviewed changes
pkg/test/context.go Outdated Show resolved Hide resolved
@joelanford
Copy link
Member

IMO, a name change because of a lint error is not sufficient reason to make a breaking change. I think this should be closed.

@camilamacedo86 camilamacedo86 changed the title rename test.TestCtx to test.Ctx rename test.TestCtx to test.Context Feb 11, 2020
estroz
estroz reviewed Feb 11, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@estroz
Copy link
Member

estroz commented Feb 11, 2020

@joelanford I agree, although this should have been Context from the start. I'm fine holding this until we discuss what we're doing with the test framework.

@camilamacedo86
Copy link
Contributor Author

Working on to apply @joe suggestion.

@camilamacedo86 camilamacedo86 changed the title rename test.TestCtx to test.Context WIP : rename test.TestCtx to test.Context Feb 11, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 11, 2020
@joelanford joelanford mentioned this pull request Feb 11, 2020
Merged
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 12, 2020
@camilamacedo86 camilamacedo86 changed the title WIP : rename test.TestCtx to test.Context Deprecated test.TestCtx in order to rename test.TestCtx to test.Context Feb 12, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2020
@camilamacedo86
Copy link
Contributor Author

Hi @joelanford it is following your suggestion now please let us know if you are ok with.

@camilamacedo86 camilamacedo86 mentioned this pull request Feb 12, 2020
Closed
joelanford
joelanford previously requested changes Feb 13, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
test/e2e/tls_util_test.go Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Contributor Author

All, solved please feel free to check. @joelanford

estroz
estroz suggested changes Feb 20, 2020
View reviewed changes
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

Do a ctrl + f for TestCtx in docs, there are still a few lines than need updating to Context.

@camilamacedo86
Copy link
Contributor Author

Screenshot 2020-02-21 at 15 09 01

Hi @estroz,

I fund just one reference which was missing and it is fixed. Could you please check now? If you still find someplace that should be updated please, let me know.

jmrodri
jmrodri approved these changes Feb 21, 2020
View reviewed changes
Copy link
Member

@jmrodri jmrodri 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 Feb 21, 2020
estroz
estroz suggested changes Feb 25, 2020
View reviewed changes
pkg/test/context.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@camilamacedo86 camilamacedo86 dismissed joelanford’s stale review February 26, 2020 10:41

Unable to check this one.

estroz
estroz suggested changes Feb 26, 2020
View reviewed changes
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
pkg/test/context.go Outdated Show resolved Hide resolved
estroz
estroz reviewed Feb 27, 2020
View reviewed changes
pkg/test/context.go Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 27, 2020
@camilamacedo86
Update pkg/test/context.go
f379fc7 
Co-Authored-By: Eric Stroczynski <[email protected]>
estroz
estroz reviewed Feb 27, 2020
View reviewed changes
pkg/test/context.go Outdated Show resolved Hide resolved
@camilamacedo86
Update pkg/test/context.go
0b0a68c 
Co-Authored-By: Eric Stroczynski <[email protected]>
estroz
estroz approved these changes Feb 27, 2020
View reviewed changes
Copy link
Member

@estroz estroz 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 Feb 27, 2020
@camilamacedo86 camilamacedo86 merged commit 453e43e into operator-framework:master Feb 27, 2020
@camilamacedo86 camilamacedo86 deleted the solve-todo-2 branch February 27, 2020 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmrodri jmrodri jmrodri approved these changes

@estroz estroz estroz approved these changes

@jmccormick2001 jmccormick2001 Awaiting requested review from jmccormick2001

@joelanford joelanford Awaiting requested review from joelanford

Assignees

@jmrodri jmrodri

@estroz estroz

Labels
lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@camilamacedo86 @joelanford @estroz @jmrodri @openshift-ci-robot