Skip to content

Fix configuration param and documentation links in CSP#11098

Merged
andreslucena merged 1 commit intodevelopfrom
fix/content-security-policy-links
Jun 28, 2023
Merged

Fix configuration param and documentation links in CSP#11098
andreslucena merged 1 commit intodevelopfrom
fix/content-security-policy-links

Conversation

@alecslupu
Copy link
Copy Markdown
Contributor

@alecslupu alecslupu commented Jun 26, 2023

🎩 What? Why?

After #10700 has been merged, the following documentation links have been published:

This PR attempt to fix all the places where the code pointed to newly published pages.
In the same time, attempts to fix a configuration issue: the system was developed to support stringified keys, yet, after the latest changes were implemented, this did not worked as expected:

# works 

    Decidim.content_security_policies_extra = {
      "img-src" => %w(https://via.placeholder.com),
    }

# does not work ( raises a RuntimeError ) 

    Decidim.content_security_policies_extra = {
      "img-src": %w(https://via.placeholder.com),
    }

📌 Related Issues

Link your PR to an issue

Testing

  1. Test the links and see they point to right locations

For the configuration:

  1. On the develop branch test the code snippets from above
  2. Pass keys as symbols to configuration
  3. See it fails
  4. Apply patch
  5. Repeat step 2
  6. See the error is gone

♥️ Thank you!

@alecslupu alecslupu force-pushed the fix/content-security-policy-links branch from dfccf14 to 5b7b8e9 Compare June 26, 2023 21:09
@alecslupu alecslupu marked this pull request as ready for review June 26, 2023 21:59
@alecslupu alecslupu added type: fix PRs that implement a fix for a bug release: v0.28 Issues or PRs that need to be tackled for v0.28 labels Jun 26, 2023
andreslucena
andreslucena approved these changes Jun 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@andreslucena andreslucena left a comment

Choose a reason for hiding this comment

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

I've just checked it and it works 👌🏽

These are always difficult to catch while on review

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jun 27, 2023

@alecslupu I've already added it on other comment in a similar issue:

This is a bug that only happens in v0.28.0.dev, so we don't need to backport it. Maybe we should change the type: fix label to type: internal?

I'm thinking on a new procedure for making easier signaling these kind of exclusions for #11065. Also, focusing in the CHANGELOG, this isn't relevant to implementers, as it's a bug introduced (and solved) in v0.28.0.dev itself, so it makes sense to not add it there.

Or do you have another idea to signal this? I guess the question would be: "how can we mark which issues we don't need to backport?"

@andreslucena andreslucena changed the title Fix configuration param and documentation links Fix configuration param and documentation links in CSP Jun 27, 2023
@alecslupu alecslupu added type: internal PRs that aren't necessary to add to the CHANGELOG for implementers and removed type: fix PRs that implement a fix for a bug release: v0.28 Issues or PRs that need to be tackled for v0.28 labels Jun 27, 2023
@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Jun 27, 2023

@alecslupu I've already added it on other comment in a similar issue:

This is a bug that only happens in v0.28.0.dev, so we don't need to backport it. Maybe we should change the type: fix label to type: internal?
I'm thinking on a new procedure for making easier signaling these kind of exclusions for #11065. Also, focusing in the CHANGELOG, this isn't relevant to implementers, as it's a bug introduced (and solved) in v0.28.0.dev itself, so it makes sense to not add it there.

Or do you have another idea to signal this? I guess the question would be: "how can we mark which issues we don't need to backport?"

We could either use “type: internal”, or we could use “type: fix” and “release: ….”

I imagine that a “type: internal” would be more suitable for pipeline changes or any other work that we as maintainers need to do in order to keep the repo running.

@alecslupu
Copy link
Copy Markdown
Contributor Author

alecslupu commented Jun 27, 2023

@alecslupu I've already added it on other comment in a similar issue:

This is a bug that only happens in v0.28.0.dev, so we don't need to backport it. Maybe we should change the type: fix label to type: internal?
I'm thinking on a new procedure for making easier signaling these kind of exclusions for #11065. Also, focusing in the CHANGELOG, this isn't relevant to implementers, as it's a bug introduced (and solved) in v0.28.0.dev itself, so it makes sense to not add it there.

Or do you have another idea to signal this? I guess the question would be: "how can we mark which issues we don't need to backport?"

@andreslucena why don’t we introduce a new type? ‘type: no-backport’ this way we may be able express exactly what we want…

@andreslucena
Copy link
Copy Markdown
Member

andreslucena commented Jun 28, 2023 •
edited
Loading

‘type: no-backport’ this way we may be able express exactly what we want…

I prefer to leave the rule of "one PR, one type", so it's easier to understand. What I introduced is the "no-backport" label, so we can have the type that we prefer (as we usually would work with) and also the "no-backport" label to signal that this PR should not be backported.

We can change it afterwards, so I'll merge this with that label, and if we find a better system it's easy to find what PR should be changed (at least easier than my original idea of messing with the "type: internal" and "type: fix" labels).

@andreslucena andreslucena added the no-backport Pull Requests that should not be backported label Jun 28, 2023
@andreslucena andreslucena merged commit a8a5140 into develop Jun 28, 2023
@andreslucena andreslucena deleted the fix/content-security-policy-links branch June 28, 2023 07:10
entantoencuanto added a commit that referenced this pull request Jul 3, 2023
@entantoencuanto
Merge branch 'redesign/sync-develop-2' into 'feature/redesign-staging'
7138f79 
* redesign/sync-develop-2: (150 commits)
  Adapt tests to redesign
  Fix stylelint offenses
  Fix linter offenses
  Fix sanitizer
  Recover deleted translation
  Recover test fix
  Fix method definition and syntax
  Check for supported locale in Emoji picker (#11079)
  Fix configuration param and documentation links in CSP (#11098)
  Show all projects if none is selected when the voting has finished (#11090)
  Add Content Security Policy support (#10700)
  Replace `bootstrap-tagsinput` npm package with `tom-select` (#11076)
  Avoid password change to be requested when user registration mode is disabled (#11070)
  Lock sass-embedded to 1.62 (#11074)
  Add a button to send a newsletter to the admin (#10896)
  Fixing more tests
  Fixing more specs
  Fixing most of the failings specs
  Fixing most of the failings specs
  Bump doorkeeper from 5.5.4 to 5.6.6 (#11002)
  ...
andreslucena pushed a commit that referenced this pull request Jul 18, 2023
@alecslupu @andreslucena
Fix configuration param and documentation links in CSP (#11098)
c909678
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreslucena andreslucena andreslucena approved these changes

Assignees

No one assigned

Labels

module: core no-backport Pull Requests that should not be backported type: internal PRs that aren't necessary to add to the CHANGELOG for implementers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alecslupu @andreslucena