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

Bugfix: github_repo does not apply defaults on existing repos #2386

Merged
merged 11 commits into from
Nov 22, 2021
Merged

Bugfix: github_repo does not apply defaults on existing repos #2386

merged 11 commits into from
Nov 22, 2021

Conversation

atorrescogollo
Copy link
Contributor

SUMMARY

Fixes #2376.

Module github_repo sets parameter default values when not specified in existing repos.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

github_repo

ADDITIONAL INFORMATION

Module github_repo is applying defaults when repo already exists. However, defaults must be set only when the repo is created. Otherwise, not specifying a parameter will apply default value, which is a wrong behaviour.

Also, two tests are include in order to ensure this behaviour:

  • test_create_new_org_repo_incomplete: new repo with default values
  • test_idempotency_existing_org_private_repo: preserve private and description on existing repos

@atorrescogollo atorrescogollo changed the title Bugfix: github_repo do not apply defaults on currently existing repos Bugfix: github_repo does not apply defaults on existing repos Apr 30, 2021
@ansibullbot
Copy link
Collaborator

cc @stpierre
click here for bot help

@ansibullbot

This comment has been minimized.

Sign in to view
@ansibullbot ansibullbot added ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed community_review labels Apr 30, 2021
@aminvakil
Copy link
Contributor

Thanks for your contribution! Please add a changelog fragment.

Also if you're changing argument_spec and set default as False instead of none, please change it in documentation too, so that sanity tests pass.

@ansibullbot ansibullbot added community_review needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Apr 30, 2021
felixfontein
felixfontein reviewed Apr 30, 2021
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

I think this PR does something else than fixing #2376. (In particular, it is a breaking change, since not specifying description no longer sets the description to an empty string, and similar for private.)

(While I prefer the new behavior, it is still a breaking change and needs a deprecation period.)

plugins/modules/source_control/github/github_repo.py Outdated Show resolved Hide resolved
plugins/modules/source_control/github/github_repo.py Outdated Show resolved Hide resolved
@ansibullbot ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI stale_ci CI is older than 7 days, rerun before merging labels Apr 30, 2021
@felixfontein felixfontein added the breaking_change This PR contains a breaking change that MUST NOT be backported label Apr 30, 2021
@felixfontein
Copy link
Collaborator

BTW, this is a PR which removes similar bad defaults in a backwards compatible way for docker_container: #1343

@atorrescogollo
Copy link
Contributor Author

In particular, it is a breaking change, since not specifying description no longer sets the description to an empty string, and similar for private.

@felixfontein

Sorry, I don't see why it is a breaking change. For PyGithub, an empty string in description is the same as description=None. In fact, #2376 is caused because the repo was created with description=''. When replaying, it is trying to compare ''==None so it sets changed=True. For private parameter, if None, PyGithub defaults to False which means there is no behaviour change.

Keeping current version of the module breaks idempotency and results are unexpected.

@atorrescogollo
Copy link
Contributor Author

@aminvakil @felixfontein This was left pending. It is not clear to me what remains to be done with this PR. The latest talk was about a breaking change but I need more information on how to deal with the deprecation period.

@felixfontein
Copy link
Collaborator

This is still a breaking change. You need to keep the default values in this PR, and (once this is merged) create a new PR which adds a force_defaults option (default value true, which means current behavior). Once that has been added, we can deprecate the default true for force_defaults and announce that it will change to false in a future version (that would happen in a third PR so we have at least one version with the force_defaults option which doesn't output a deprecation warning).

@ansibullbot ansibullbot removed needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_maintainer labels Sep 13, 2021
@russoz
Copy link
Collaborator

russoz commented Nov 13, 2021

hi @atorrescogollo are you still planning on working on this?

needs_revision

@russoz russoz added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 13, 2021
@ansibullbot ansibullbot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Nov 13, 2021
@ansibullbot ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Nov 14, 2021
@atorrescogollo
Copy link
Contributor Author

Hi @russoz and @felixfontein , now the same behaviour is given with force_defaults enabled (default behaviour) in order to deal with breaking change. When disabled, it won't change description and private attributes.

Next step should be to change the default behaviour of force_defaults to disabled in other PR.

@ansibullbot ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Nov 18, 2021
@atorrescogollo @felixfontein
Update plugins/modules/source_control/github/github_repo.py
d57e154 
Co-authored-by: Felix Fontein <felix@fontein.de>
felixfontein
felixfontein approved these changes Nov 20, 2021
View reviewed changes
Copy link
Collaborator

@felixfontein felixfontein left a comment

Choose a reason for hiding this comment

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

LGTM! Will merge by Monday if nobody complains.

@felixfontein felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. and removed check-before-release PR will be looked at again shortly before release and merged if possible. labels Nov 20, 2021
@felixfontein felixfontein merged commit 17c3708 into ansible-collections:main Nov 22, 2021
@patchback
Copy link

patchback bot commented Nov 22, 2021 •
edited
Loading

Backport to stable-4: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-4/17c3708f31631ce6a10c39ccc0ef3405feb94f04/pr-2386

Backported as #3769

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Nov 22, 2021
@atorrescogollo
Bugfix: github_repo does not apply defaults on existing repos (#2386)
e679ca7 
* github_repo do not apply defaults on currently existing repos

* Fixed sanity

* Fixed doc defaults

* Added changelog

* Fix "or" statement and some formatting

* Improve description change check

* Added api_url parameter for unit tests and default values for private and description parameters

* Added force_defaults parameter

* Improved docs

* Fixed doc anchors for force_defaults parameter

* Update plugins/modules/source_control/github/github_repo.py

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 17c3708)
@patchback patchback bot mentioned this pull request Nov 22, 2021
Merged
@felixfontein
Copy link
Collaborator

@atorrescogollo thanks for improving this!
@russoz @aminvakil thanks for reviewing!

felixfontein pushed a commit that referenced this pull request Nov 22, 2021
@patchback @atorrescogollo
Bugfix: github_repo does not apply defaults on existing repos (#2386) (…
3eab4fa 
…#3769)

* github_repo do not apply defaults on currently existing repos

* Fixed sanity

* Fixed doc defaults

* Added changelog

* Fix "or" statement and some formatting

* Improve description change check

* Added api_url parameter for unit tests and default values for private and description parameters

* Added force_defaults parameter

* Improved docs

* Fixed doc anchors for force_defaults parameter

* Update plugins/modules/source_control/github/github_repo.py

Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>
(cherry picked from commit 17c3708)

Co-authored-by: Álvaro Torres Cogollo <atorrescogollo@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aminvakil aminvakil aminvakil left review comments

@russoz russoz russoz left review comments

@felixfontein felixfontein felixfontein approved these changes

Assignees
No one assigned
Labels
breaking_change This PR contains a breaking change that MUST NOT be backported bug This issue/PR relates to a bug community_review module module plugins plugin (any type) source_control tests tests unit tests/unit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

github_repo always reports changed status
5 participants
@atorrescogollo @ansibullbot @aminvakil @felixfontein @russoz