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

Relax Stickiness Rules for Strains with Conditions #254

Closed
trieloff opened this issue Feb 25, 2020 · 4 comments · Fixed by #253
Closed

Relax Stickiness Rules for Strains with Conditions #254

trieloff opened this issue Feb 25, 2020 · 4 comments · Fixed by #253
Assignees
Labels
enhancement New feature or request

Comments

@trieloff
Copy link
Contributor

Previously, we assumed a strain with a url was not sticky, but a strain with a condition was. Now, that URLs have moved below conditions, every strain with a condition is sticky, even if it wasn't before.

I suggest we relax the stickiness rules so that the condition will actually be evaluated for stickiness.

Based on the list of conditions here, I'd suggest:

sticky

  • referer
  • url_param

** conditionally sticky** (sticky when at least one component condition is sticky)

  • and
  • or
  • not

not sticky

  • everything else
@dominique-pfister
Copy link
Contributor

dominique-pfister commented Feb 26, 2020

Looking at the existing tests in helix-shared a string condition is considered rendering the strain sticky as well, e.g. this strain:

  - <<: *basestrain
    name: pipeline
    condition: req.http.host == "pipeline.project-helix.io"
    params:
      - search
      - lang
    content:
      repo: hypermedia-pipeline
      ref: master
      owner: adobe
    static:
      <<: *myrepo
      path: /www
    redirects:
      - from: (.*)\.php
        to: $1.html
      - from: /content/dam/(.*)$
        to: /htdocs/$1

is expected to have canonical form:

  - code:
      protocol: https
      host: github.com
      port: ""
      hostname: github.com
      owner: adobe
      repo: project-helix.io
      ref: master
      path: ""
    content:
      protocol: https
      host: github.com
      port: ""
      hostname: github.com
      owner: adobe
      repo: hypermedia-pipeline
      ref: master
      path: ""
    static:
      magic: false
      allow: []
      deny: []
      protocol: https
      host: github.com
      port: ""
      hostname: github.com
      owner: adobe
      repo: project-helix.io
      ref: master
      path: /www
    directoryIndex: index.html
    package: ""
    name: pipeline
    sticky: true
    condition: req.http.host == "pipeline.project-helix.io"
    perf:
      device: ""
      location: ""
      connection: ""
    urls: []
    params:
      - search
      - lang
    redirects:
      - from: (.*)\.php
        to: $1.html
      - from: \/content\/dam\/(.*)$
        to: /htdocs/$1

What do you think, @trieloff : should I rework the stickyness in the expected output(s) or assume string-based conditions are always sticky, which as mentioned would make all tests pass?

@trieloff
Copy link
Contributor Author

I'd phrase the old policy as: "when in doubt, treat it as sticky". Because we couldn't understand string based conditions, they were always sticky.

With the new condition expressions, we can be more precise, and treat a condition like req.http.host == "pipeline.project-helix.io" as non-sticky, as it does not rely on either referrer or cookies or URL parameters.

@dominique-pfister
Copy link
Contributor

So, I remove the sticky: true in the expected output?

@trieloff
Copy link
Contributor Author

Yes.

trieloff added a commit that referenced this issue Feb 28, 2020
BREAKING CHANGE: The 5.3.1 release introduces breaking changes for conditions handling. This commit formally acknowledges that.

see #256 #253 #254
adobe-bot pushed a commit that referenced this issue Feb 28, 2020
# [6.0.0](v5.3.1...v6.0.0) (2020-02-28)

### Documentation

* **changelog:** mark 5.3.1 as a breaking change ([b556e6e](b556e6e)), closes [#256](#256) [#253](#253) [#254](#254)

### BREAKING CHANGES

* **changelog:** The 5.3.1 release introduces breaking changes for conditions handling. This commit formally acknowledges that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants