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

(Initial investigation) PR #1161 breaks all controlrepos that use control_branch #1173

Closed
dylanratcliffe opened this issue Jun 25, 2021 · 2 comments
Closed

(Initial investigation) PR #1161 breaks all controlrepos that use control_branch #1173

dylanratcliffe opened this issue Jun 25, 2021 · 2 comments
Labels
bug

Comments

@dylanratcliffe
Copy link

As of the current version the R10K::Puppetfile#add_module method does the following which was introduced in #1161 :

    if @default_branch_override != nil
      args[:default_branch_override] = @default_branch_override
    end

This means that all module classes will have the :default_branch_override argument set if the R10K::Puppetfile#load was called with a non-nil default_branch_override. The :default_branch_override option is file for R10K::Module::Git modules but not for `R10K::Module::Forge for example and leats to deployment errors such as:

2021-06-25T14:07:54.515+02:00 INFO  [qtp1153833797-509] [p.c.app] Queuing deploy for production_cdpe_ia_1624622874129
2021-06-25T14:07:54.530+02:00 INFO  [deploy-pool-1] [p.c.core] Attempting to deploy :environment 'production_cdpe_ia_1624622874129'...
2021-06-25T14:07:55.225+02:00 ERROR [deploy-pool-1] [p.c.core] Errors while deploying environment 'production_cdpe_ia_1624622874129' (exit code: 1):
/opt/puppetlabs/puppet/lib/ruby/gems/2.7.0/gems/rugged-0.27.7/lib/rugged/repository.rb:258: warning: Using the last argument as keyword parameters is deprecated
ERROR	 -> Failed to evaluate /etc/puppetlabs/code-staging/environments/production_cdpe_ia_1624622874129/Puppetfile
Original exception:
R10K::Module::Forge cannot handle option 'default_branch_override'
2021-06-25T14:07:55.406+02:00 INFO  [deploy-pool-1] [p.c.core] Finished deploy attempt for :environment 'production_cdpe_ia_1624622874129'.

Steps to Reproduce

I won't have time to do a replication case but I suspect any Puppetfile that contains something like this will cause it:

mod 'puppetlabs/sqlserver',                   :latest

mod 'someone/something',
    git: 'git@somewhere.com/whatever',
    branch: :control_branch,
    default_branch: 'main'

Environment

PE 2021.2.0

Additional Context

This is in a released version of PE and will affect a great many customers. Let's get onto this ASAP
@Magisus
Copy link
Collaborator

Magisus commented Jun 25, 2021

Thanks Dylan, we're taking a look!

justinstoller added a commit to justinstoller/r10k that referenced this issue Jun 25, 2021
@justinstoller
(puppetlabs#1173) Allow default_branch_override argument for all modu…
49bb17a 
…le types

In 1ff9995, we moved the setting of the `default_branch_override` from
all module declarations that were Hashes (ie, not Puppetfile declared
Forge modules) to all modules. This broke Forge modules for users who
pass the `default_branch_override` flag.

It should be noted that code-wise, users who set the
`default_branch_override` flag when deploying YAML based environments
(which always use Hash based module declarations) or had SVN modules in
their Puppetfile (also Hash based) would have failed. However, those
scenarios are either not supported (in the case of YAML based envs) or
essentially unused (in the case of SVN).

Other options to fix this regretion include:
  * Have the caller of Module.new first determine if the module will be
  a Git module and then conditionally add the option, or
  * Have the caller of Module.new set the default_branch_override after
  creating the module, if the module responds to the setter

Both of these options involve the caller having to introspect enough
about the type of module being created that it destroys what value we
get from the Module.new facade. Consequently, this patch takes the
approach to loosen Module argument strictness, allowing potentially
unused options to passed to any module.
justinstoller added a commit to justinstoller/r10k that referenced this issue Jun 25, 2021
@justinstoller
(puppetlabs#1173) Allow default_branch_override argument for all modu…
37c456b 
…le types

In 1ff9995, we moved the setting of the `default_branch_override` from
all module declarations that were Hashes (ie, not Puppetfile declared
Forge modules) to all modules. This broke Forge modules for users who
pass the `default_branch_override` flag.

It should be noted that code-wise, users who set the
`default_branch_override` flag when deploying YAML based environments
(which always use Hash based module declarations) or had SVN modules in
their Puppetfile (also Hash based) would have failed. However, those
scenarios are either not supported (in the case of YAML based envs) or
essentially unused (in the case of SVN).

Other options to fix this regretion include:
  * Have the caller of Module.new first determine if the module will be
  a Git module and then conditionally add the option, or
  * Have the caller of Module.new set the default_branch_override after
  creating the module, if the module responds to the setter

Both of these options involve the caller having to introspect enough
about the type of module being created that it destroys what value we
get from the Module.new facade. Consequently, this patch takes the
approach to loosen Module argument strictness, allowing potentially
unused options to passed to any module.
mwaggett added a commit that referenced this issue Jun 25, 2021
@mwaggett
Merge pull request #1174 from justinstoller/uncontrolled_branches
7656f77 
(#1173) Allow default_branch_override argument for all module types
@Magisus
Copy link
Collaborator

Magisus commented Jun 29, 2021

The fix for this has been merged and released as 3.9.3.

@Magisus Magisus closed this as completed Jun 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Magisus @dylanratcliffe