Allowing file-specific transformation scripts for PSyclone

This pull request is related to Issue https://github.com/hiker/fab/issues/18 and the pull request on hiker fab: hiker#19. The purpose is to allow each x90 file to have its specific transformation script.
(1) The changes are mainly in:
source/fab/steps/psyclone.py:
The psyclone function now takes in a function as the argument for the transformation_script parameter. This input function will be written by the user to take in an x90 file path (parameter fpath) and the config, and return a transformation script path.
(2) The related tests have also been updated:
tests/system_tests/psyclone/test_psyclone_system_test.py:
Tests have been added to verify that the input transformation script function takes in the x90 file path and returns a transformation script path, which is then passed successfully to the psyclone command with -s. The checking for the transformation_script_hash is now done in the unit test, as now the transformation_script_hash is calculated when processing each file.
tests/unit_tests/steps/test_psyclone_unit_test.py:
Transformation_script_hash test and prebuild_hash test have been updated.
(3) Documentations and examples have been updated:
docs/source/writing_config.rst:
A PSyclone step is added to this config writing documentation to demonstrate how to use this step.
run_configs/lfric/atm.py and run_configs/lfric/gungho.py:
The atm and gungho examples have been updated to show examples of how a get_transformation_script function can be defined and then passed to psyclone.

I've had a look through this change and the goal of providing special cases to the transformation script is certainly a functional requirement so I have no problem with that.

Thanks!

My question is around the implementation. Why a callback function? It seems to add complexity and I'm not sure what's being won by its use.
Why not pass a path to a global script (optional because we may not want transformations) and a dictionary of special cases (also optional because we may have none).

This implementation allows the same usage, just passing in the global script, so we are not losing anything. A dictionary could be useful, but it can also be used in the function being provided. So, functionality-wise this approach can support a dictionary of special cases.
But, if a Fab script uses this dictionary, it means that adding a new file-specific PSyclone script to the optimisation directory in LFRic would mean that we then need to modify the Fab script as well (adding this new script to the dictionary), while it is not necessary with the example functions provided - the function will dynamically detect any scripts as required, so we have the same behaviour as the current LFRic build system.
Providing a function means any typo in the function name is discovered much earlier, either when you import the function, or when you call the PSyclone step ... way before it would be called. And typing means we have a proper typed variable (as opposed to a string or path, which can be a function taking any kind of parameters). And the implementation is much more straightforward (no importlib or exec or import or whatever you use). Also, syntax errors in the script are detected much earlier (immediately when importing, not deep down the fab call tree).
The LFRic Fab scripts are based on an OO-design - which means the base class will provide the same behaviour the old build-systems has (checking for a python script with the same local path and base name as the x90 file), and all application-specific build scripts don't need to deal with this at all - the base class will set its own method to be used. So in LFRic a typical user script will never see this function.

The only reason I can think of for the callback is if the mapping it performs is contingent on the configuration and off the top of my head I can't think of a situation where that would be so.

The config object is required to properly create relative path names (in case that more directory levels are added to some of the more complex LFRic apps). And we also intend at some stage that the config object stores information whether OpenMP is enabled or not. This is required, since Fab will have to trigger fparser to handle or ignore omp conditional sentinels in any source code during analysis - and it will then also be able to enable/disable this in the (new and upcoming) compiler objects independent of the compiler. Meaning the configuration will be simpler. Which also means, a PSyclone scripts can query the config object for OpenMP,  and if it is disabled not apply any OpenMP transformations at all.

Actually, there is one additional reason why I would prefer more flexibility: as part of NG-ARCH (UM-physics to non-CPU architectures), we will be using PSyclone on UM files (i.e. f90 files). I am not really certain at this stage what kind of scripting we will end up with (one script for all of UM?? Unlikely, but who knows). Same approach as lfric? Again, having a function will give us the required flexibility

Hi @MatthewHambley , thanks for the review. I would second @hiker's opinions. For me, implementing this with a callback function would have the following benefits:
(1) Matching the current LFRic build system behaviours:
LFRic_atm currently has two file-specific optimisation scripts that uses the mapping from .x90 files to .py files: https://code.metoffice.gov.uk/trac/lfric_apps/browser/main/trunk/applications/lfric_atm/optimisation/meto-xc40/algorithm
(2) Allowing flexibility for users without needs to touch the FAB repo:
With the callback function, users can define their own logics for getting optimisation scripts by simply changing this function in the app build scripts managed by themselves. They can choose to just use the mapping method given in the example or use a dictionary for matching optimisation scripts to x90 files or use other logics for future applications.
(3) Fitting into the LFRic Fab OO-design scripts:
This callback function will be a method of the LFRic Fab OO build script base class, along with several other methods. So it won't be too special or complex for users. Users can choose to just use it by default without touching it or overwrite it in the app-specific build scripts.

I must be missing something because I don't see how the callback you are describing provides any more functionality.
The logic for compiling the list of optimisation functions must exist somewhere and I don't see how, functionally, it makes any difference whether the list is compiled dynamically as need (callback) or up front at initialisation. Unless you are worried that it will get very large and become a burden on memory.
Ignoring what happens in the Fab framework and looking just at the user's build script, this is what I think this change provides:
def discover_transformation_script(algorithm: Path) -> Path:
    Synthesize transformation script path
    If file exists, return it, otherwise return global script.
...
fab_psyclone_step(discover_transformation_script)

And this is what I'm suggesting:
def discover_transformation_scripts(root: Path) -> Dict[Path, Path]:
    Iterate over transformation scripts:
        Add mapping from algorithm to transformation
    return mappings
...
fab_psyclone_step(global_transformation_script_path, discover_transformation_scripts(...)

I don't see any difference in where (who has responsibility) the dynamic content is being generated.
So what have I missed. What does the callback give that a static look-up table doesn't? Both generate their data dynamically and client side. Neither require modifications to the Fab framework when a new transformation is added.

OK, I understand you know, I thought you were talking of a static mapping.
I had a chat with Jason today. The two approaches seems to be identical in what can be done with them, with slightly different run time behaviour, which will de-facto not matter (you call the function once in serial, but need to traverse the whole directory. The approach her calls it for each algorithm file, but in parallel and only checks for existence of two files). All other functionality that one offers can be done with the others as far as we can see.
So the main question is: is there anything wrong with this approach?

Having thought about it some more and conferred with colleagues I have been won round to the callback approach. So I will proceed with the review on that basis.


Stack Reports Up to Date but Custom Catalog has Higher Versions

Rancher Versions:
Server: 1.3.2
Docker Version:
1.10.3
OS and where are the hosts located? (cloud, bare metal, etc):
Ubuntu 14.04 LTS, AWS EC2
Setup Details: (single node rancher vs. HA rancher, internal DB vs. external DB)
single node rancher with external host agent
Environment Type: (Cattle/Kubernetes/Swarm/Mesos)
Cattle
After upgrading to rancher server version 1.3.2, a few of our existing stacks that had available upgrades now report "up to date". If you browse to the catalog there are higher versions available, however if you click on the "up to date" link it brings you to an upgrade page with only the current version in the drop down.
Thoughts?

Without seeing the catalog, my first guess would be the versions that don't show up have min_rancher_version  and/or max_rancher_version that are not satisfied by 1.3.2.

Thanks for the response vincent. Would these attributes be in the rancher_compose definition? We do not explicitly set those in there.

They go in the .catalog/catalog section (along with question definitions) and can technically be in either file.
Also depending on what version you're upgrading from, at some point between 1.1 and 1.2 we switched from considering any higher folder number an "upgrade" to semver comparison of the version of the templates vs the one currently installed.  So those versions need to be a) present and b) increasing.

In both working (says upgrade available) and non working stacks (says "up to date") the version attribute is defined in rancher compose in the .catalog section:
Working (deployed version = 0.14.43):
.catalog:
name: "test-stack"
version: "0.15.45"
description: "blah blah"
uuid:
Not Working (deployed version = <IP_ADDRESS>):
.catalog:
name: "AnotherTestStack"
version: "<IP_ADDRESS>"
description: "blah blah"
uuid:

In addition, if we deploy the same version of that stacks that wont show up as potential upgrade targets, the questions are not flowing through to the environment variables in the container. Is it fair to say this is a stack definition issue despite the fact that the catalog load process does not complain about them?

https://github.com/rancher/rancher-catalog-service/blob/master/manager/catalog_manager.go
In this manager,  the 4th position of the version is ignored in getVersionFromRancherCompose(). I find it misleading that the catalog entry would load without warning when part of the version is ignored in comparisons.

Semver  is major.minor.patch only.

Thanks for that info. Perhaps there is some way to make this clear to the end user. While browsing a catalog with 4 part versions it's not clear that <IP_ADDRESS> can't be upgraded to <IP_ADDRESS>. Just a thought... to prevent the same confusion I went through, even if it was partly due to my own ignorance.


Missing APK for release 2.8

Hi there,
You didn't attach the APK on the 2.8 release assets.
Could you please attach it?
Thank you

Yes, missed that APK. Will do later

APK added now.


Fix playing of undefined patterns

Some XM modules contain a pattern table that references an undefined
pattern (whose index is higher than the number of patterns). Both
ft2-clone and MilkyTracker agree that those are empty patterns of 64
rows and play them accordingly.
This commit does the same in libxm, avoiding a segfault.

Example of such module, see last pattern
polyamber.zip

Thanks for the contribution. Are we using the same libxm base? Because it doesn't segfault on undefined patterns (see https://github.com/Artefact2/libxm/blob/340033a7b0d56ccec329bb2c1fccc264ad57fe44/src/load.c#L60).
Your fix is better than just discarding them, as it conforms to what trackers do, but as-is it does nothing on the current codebase. I also changed xm_get_number_of_rows() so that players can report more accurate position information. See 68bd967 and 94b5121.

I think that happens because I don't use XM_DEFENSIVE.
Do we need a compile time macro for that BTW? The fixes I have provided over the months were all in the direction of sanitizing against weird modules, which I think it can be done efficiently without guarding them with a compile time macro.

The original idea was to save a few bytes in demos, where you know the module you're loading is OK. Either way the undefined pattern stuff is now there regardless of that compile-time flag.


fix(ci): 'on' in django.yml

:tada: This PR is included in version 1.0.0 :tada:
The release is available on GitHub release
Your semantic-release bot :package::rocket:


integrate comet-lang

starting from v3, https://github.com/vivid-planet/comet-lang will be used for translation (https://github.com/vivid-planet/comet-admin-lang will stay as public archive for backwards compatibility)
once v3 is released, the translation pipelines should run on main rather than next

@kaufmo please review.


inclusion of "solidity-cli" causes many circular dependency warnings

Without "solidity-cli" in my code (I'm using ESM with the .mjs file extension):
node --version
v14.16.1

npm run test -- --match="*if binary is same*"

><EMAIL_ADDRESS>test
> ava "--match=*if binary is same*"

  if binary is same as solcli bytecode

  Rejected promise returned by test. Reason:

  ReferenceError {
    message: 'compiled is not defined',
  }

  › file://test/eth_test.mjs:11:15

  ─

  1 test failed

With "solidity-cli" included in my code:
npm run test -- --match="*if binary is same*"

><EMAIL_ADDRESS>test
> ava "--match=*if binary is same*"

(node:66193) Warning: Accessing non-existent property 'cat' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:66193) Warning: Accessing non-existent property 'cd' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'chmod' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'cp' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'dirs' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'pushd' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'popd' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'echo' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'tempdir' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'pwd' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'exec' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'ls' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'find' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'grep' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'head' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'ln' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'mkdir' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'rm' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'mv' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'sed' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'set' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'sort' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'tail' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'test' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'to' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'toEnd' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'touch' of module exports inside circular dependency
(node:66193) Warning: Accessing non-existent property 'uniq' of module exports inside circular dependency
...
  if binary is same as solcli bytecode

  Rejected promise returned by test. Reason:

  ReferenceError {
    message: 'compiled is not defined',
  }

  › file://test/eth_test.mjs:11:15

  ─

  1 test failed

For your convenience, I added --trace-warnings to show you the first result:
npm run test -- --match="*if binary is same*" --node-arguments="--trace-warnings"

><EMAIL_ADDRESS>test
> ava "--match=*if binary is same*" "--node-arguments=--trace-warnings"

(node:68180) Warning: Accessing non-existent property 'cat' of module exports inside circular dependency
    at emitCircularRequireWarning (internal/modules/cjs/loader.js:650:11)
    at Object.get (internal/modules/cjs/loader.js:664:5)
    at Object._register [as register] (/Users/user/Projects/bla.project/node_modules/shelljs/src/common.js:453:12)
    at Object.<anonymous> (/Users/user/Projects/bla.project/node_modules/shelljs/src/cat.js:4:8)
    at Module._compile (internal/modules/cjs/loader.js:1063:30)
    at Module._extensions..js (internal/modules/cjs/loader.js:1092:10)
    at Object.require.extensions.<computed> [as .js] (/Users/user/Projects/bla.project/node_modules/ava/lib/worker/dependency-tracker.js:42:4)
    at Module.load (internal/modules/cjs/loader.js:928:32)
    at Function.Module._load (internal/modules/cjs/loader.js:769:14)
    at Module.require (internal/modules/cjs/loader.js:952:19)

Can you create a repo to reproduce this?
I do not understand what you mean with With "solidity-cli" included in my code:


Remove duplicated code in http2 examples

Motivation:
When working on some http2 related code I noticed that there is some duplication in the http2 examples. We should remove it.
Modifications:
Remove duplicated code and just extend classes to share the implementation.
Result:
Less code to maintain.

@ejona86 fair enough... I am not strong on this so lets close it.


Memory use optimizations

Reduce default size of Rhino thread scoop pool in CSSModuleBuilder
Help out GC by pro-actively clearing list/map objects when no longer needed instead of waiting for it to be done in the finalize methods.
Eliminate some potential circular references
Add serializecache console command to provide for cache serialization on demand (helps with debugging memory utilization related to caching).

OpenNTF » JavascriptAggregator #734 SUCCESS
This pull request looks good
(what's this?)

Overall the changes look good.  I think it would be good to (either now or in the future) update the method referenced above so that the changes you made that wrap it are closer together.  Possibly moving the logic between to an easy to unit test method.

OpenNTF » JavascriptAggregator #735 SUCCESS
This pull request looks good
(what's this?)

OpenNTF » JavascriptAggregator #736 SUCCESS
This pull request looks good
(what's this?)

OpenNTF » JavascriptAggregator #737 SUCCESS
This pull request looks good
(what's this?)


Adds flag '--tab-size=NUMBER / -s NUMBER'

for setting the desired size of the tab charachter ('\t'), it defaults
to 8 for backwards compatibility.

I'm sorry I haven't fixed this yet, I'll get to it as soon as possible.

Now it generates the type signature and gives an appropriate error when an invalid integer is used.

$ alex -s 1d Lexer.x
alex: -s/--tab-size option is not a valid integer

Thanks!


Remove deprecated classes and methods (#158)

First PR to start cleanup for 8.0.

Coverage increased (+0.6%) to 63.959% when pulling b9d7294a554d51d47c90bd97c5c81a4d5bb01e3b on tomwhite:remove-deprecated into e4224ab7b3b1ecc463e34252ab340f223d8e3acb on HadoopGenomics:master.


Change in rerun behavior in 7.8.0

Snakemake version

5.8.1 vs 7.8.0
Describe the bug

Snakemake v 7.8.0 (also 7.6.2) tries to re-run rules whose output files already exist. The same (expected) behaviour is not observed on snakemake version 5.8.1`
Logs

The commands below describe what I am observing
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake -v
5.8.1
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake --reason -np -s Snakefile --cluster '/opt/sge/bin/lx-amd64/qsub -q {QUEUE} -e {log} -o {log} -j y' -j 96 --jobname '{params.jobname}.{rule}.{jobid}' --keep-going --latency-wait 300 all
Building DAG of jobs...
Nothing to be done.
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ conda remove snakemake
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ mamba install -c bioconda snakemake=7.8.0
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake -v
7.8.0
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake --reason -np -s Snakefile --cluster '/opt/sge/bin/lx-amd64/qsub -q {QUEUE} -e {log} -o {log} -j y' -j 96 --jobname '{params.jobname}.{rule}.{jobid}' --keep-going --latency-wait 300 all
# the whole workflow is printed!

Minimal example

Just try to re-run a finished workflow using the two aforementioned runs. What I am observing is:

v 5.8.1: expected behaviour i.e. rules whose output files already exist are not re-run
v 7.8.0: snakemake tries to re-run the whole workflow, even though output files already exist

Additional context

I suspect this is to do with the change of behaviour caused by #1663. Specifically, the change to rerun triggers, which are now ["mtime", "params", "input", "software-env", "code"] by default. Previously they were just mtime. So to get the same functionality, I believe you need to run snakemake with --rerun-triggers mtime.

I also do not like this change, and snakemake does unnecessary reruns because of apparent changes of the software stack. I tried it out yesterday and could not understand why it determined that the software stack changed, because I think it didn't- it must have been the conda environment that snakemake automatically builds but it is totally unclear to me why it determined everything needs to rerun. I agree this should not be the new default.

My 2p- I can see why this change can be annoying and it's going to cause some headaches. However, I would say this is the "correct" behavior and it should be the default since changes in code and params are just as important as changes in input.

I haven't tried it that much, but the "code changes" seem heavy handed where non-functional changes (renaming variable, deleting whitespace, etc) still would trigger a rerun. Input changing would logically affect all the way down the DAG, but  some of the other changes may have more exceptions than cases. Probably from a "best practices for reproducibility" perspective this is correct, but who is perfect...

@ASLeonard I think it's impossible for snakemake (or any workflow manager) to establish which changes are functional and which can be ignored so they went for the heavy-handed approach. I believe that If you are sure that changes are irrelevant you can use the --touch option to update the existing output without re-running the code. So to recap and if I'm not mistaken:

Emulate behavior like < 7.8.0:

snakemake --rerun-triggers mtime ...

Update output files to override irrelevant changes in code:

snakemake --touch ...

I hear you folks, indeed, this is quite a disruptive change. I tried to make it self-explanatory, in the sense that Snakemake should print a message at the end of a dryrun, saying this:
Some jobs were triggered by provenance information,
see 'reason' section in the rule displays above.
If you prefer that only modification time is used to
determine whether a job shall be executed, use the command
line option '--rerun-triggers mtime' (also see --help).

I also agree about cosmetic nature such changes often might have, in particular during development. On the other hand, I thought the conservative approach would be a reasonable default from a reproducibility perspective. Believe me, I have heard as many requests on having the current behavior than complaints I hear now.
I personally would use it as follows: during heavy development of a workflow, I might stick to --rerun-triggers mtime, and later, I would use the default. Whenever I am sure about a particular change being no problem I would use snakemake --cleanup-metadata <filename>, which will remove all the metadata which triggers the rerun.

I hear you Johannes, this is indeed a tricky case, and good arguments for "both" sides. I agree with your judgement and recommendation, and you summarize it well - dev vs non-dev. The metadata cleanup is also a nice trick. Maybe your last reply could be a new FAQ related to the the new rerun-triggers as a general guideline. The snakemake message is actually clear, I think I simply missed it the first time I run 7.8. Thanks for the effort and this change which - in reproducibility terms - is totally useful but tougher in practise - As mentioned before, if someone does some small changes to the syntax like renaming variables, which doesnt hange the results but only the readability of the code, would that trigger a rerun?

Actually, I have seen that the message I mentioned above was accidentally not shown in 7.8.0. I will fix this now in PR #1686 along with other things. Sorry for that, I guess this made it much more disruptive than intended, together with the other bugs fixed in that PR. We urgently need a test coverage checker for Snakemake, but this is not so trivial because of the multi process nature of Snakemake. I will make that a priority though.

Also, I think when changing from snakemake < 7.8 to 7.8 and above, there shouldnt be any rerun because of the software stack and the other new triggers, only mtime should be checked as this was the only check before 7.8. Once this is done "once", the other triggers can then be checked and integrated.

Yes, that could help with the transition, however, I currently don't have the capacity to work on such a corner case feature which will become obsolete after a few days or weeks.

Is it really a corner case, when thousands of users switch to >=7.8 and experience the proposed reruns of their pipelines? It affects every use, doesnt it, when upgrading eventually? Many did not upgrade yet and are not even aware of this yet. I dont feel this is a corner case, quite the opposite actually :)

Sorry for the wording, I did not mean to put your request down. I meant corner case in terms of the period of time where this applies, compared to the amount of work to implement something like that. We would need to somehow track whether this is the first 7.8 run or not, and change the logic based on that. And then, the question is what would really be the benefit, because after the first run you would still suddenly see the changed behavior. And this might then be quite unexpected for people because they will start complain that snakemake suddenly behaves different. Hence, one would need to document this change in behavior very boldly. But still my experience is that there will always be cases where such documentation (or also messages at runtime) is/are missed by people.
So overall, I think it is not worth the effort.

All good, I didnt interpret your reply in any negative way. I am just putting on purely the "user glasses" here, abstracting from anything else, effort in particular because I am not sure how difficult such a feature would actually be.
Just proposing ideas to make the minimal impact on users switching over and not "wanting" to rerun their pipeline the first time they run v7.8 because of the new triggers, at least one of which would been triggered automatically, or this is actually not true? For me at least, this was the behavior, but maybe this was just a side effect of the conda-related software stack bug you started fixing. If that is the case, then nevermind my whole comment :)
So in short: How snakemake reacts when v7.8 is run for an existing (pre-7.8) .snakemake folder, the metadata for the software stack etc is not yet available, and the pipeline is restarted? Would it trigger anything because of that?

missing metadata means no trigger at all. And yes, the behavior you have seen is likely just the bug. I am really angry on me that I did not detect that bug beforehand. I thought that I had tested it really carefully, but apparently I missed a pretty obvious case. I don't know how that happened, and it led to exactly the situation I wanted to avoid.

Oh I see, then disregard all my comment and suggestions :). Dont worry, these things happen, particularly for a complex software just as snakemake. Once fixed, it will be ok. I was just confused because I didnt understand it was a bug when I first tested it and reported it.

On the other hand, I thought the conservative approach would be a reasonable default from a reproducibility perspective. Believe me, I have heard as many requests on having the current behavior than complaints I hear now.

There is no way to keep both parties happy with an option like this. As much as I don't like it, the current default probably is best - i.e., the more conservative approach.

There is now 7.8.1, which fixes many small glitches that occurred with the change. In particular, the software env trigger should now not lead to completely wrong false positives anymore.

Closing this now, in favor of the summarizing issue #1694.

I hit this new behaviour and opened a question on StackOverflow without knowing there was a discussion here: https://stackoverflow.com/questions/72508225/how-to-ignore-snakemakes-params-have-changed-since-last-execution?noredirect=1#comment128119555_72508225
Maybe someone could write a good answer over there explaining common workarounds?
I didn't see the message mentioned by Johannes, maybe I never made it to the end of the dryrun section. It may be useful to add it to the top, too. I pipe dryrun into less and usually just skim the top of the 10k line output.

Snakemake version
5.8.1 vs 7.8.0
Describe the bug
Snakemake v 7.8.0 (also 7.6.2) tries to re-run rules whose output files already exist. The same (expected) behaviour is not observed on snakemake version 5.8.1`
Logs
The commands below describe what I am observing
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake -v
5.8.1
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake --reason -np -s Snakefile --cluster '/opt/sge/bin/lx-amd64/qsub -q {QUEUE} -e {log} -o {log} -j y' -j 96 --jobname '{params.jobname}.{rule}.{jobid}' --keep-going --latency-wait 300 all
Building DAG of jobs...
Nothing to be done.
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ conda remove snakemake
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ mamba install -c bioconda snakemake=7.8.0
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake -v
7.8.0
(ont37) [ngs@vngs20x ~/installed/snakemake/denovoAssembly_ONT]$ snakemake --reason -np -s Snakefile --cluster '/opt/sge/bin/lx-amd64/qsub -q {QUEUE} -e {log} -o {log} -j y' -j 96 --jobname '{params.jobname}.{rule}.{jobid}' --keep-going --latency-wait 300 all
# the whole workflow is printed!

Minimal example
Just try to re-run a finished workflow using the two aforementioned runs. What I am observing is:

v 5.8.1: expected behaviour i.e. rules whose output files already exist are not re-run
v 7.8.0: snakemake tries to re-run the whole workflow, even though output files already exist

Additional context

How to solve it? I find it is so hard to use when I update the snakemake


[MLS Spec change] Editorial (#656)

Link to the exact changes
https://github.com/mlswg/mls-protocol/pull/656
Description of the changes

Not strictly editorial despite the name.

The only technical change in here is to make the variable-length TLS encoding is safe. This will be tackled as part of #882.


Istio Operator chart overlapping labels (when running side by side)

Bug Description
In order to run a canary upgrade I want to run two istio operators in the istio-operator namespace. I've noticed that the pod's labels have no version information like so:
deployment.yaml (excerpt):
[...]
kind: Deployment
[...]
spec:
  replicas: 1
  selector:
    matchLabels:
      name: istio-operator
  template:
    metadata:
      labels:
        name: istio-operator

so the matchLabels is only for istio-operator . This poses a problem for example with the endpoints:
$ kubectl -n istio-operator get endpoints
NAME                    ENDPOINTS                             AGE
istio-operator-1-11-2   <IP_ADDRESS>:8383,<IP_ADDRESS>:8383   13d
istio-operator-1-13-3   <IP_ADDRESS>:8383,<IP_ADDRESS>:8383   2h

Version
latest chart
1.13.3

Additional Information
No response

@palmobar can you please post the detailed steps that you took to reproduce this error? Otherwise it is a bit difficult to see where the problem is. Thanks.


I can't login

🐛 Describe the bug
When I click Login with Github, the new page can't be loaded correctly and the redirect_uri=http://localhost:54321/callback
The error in the page is:

The page isn't working
localhost didn't send any data.
ERR_EMPTY_RESPONSE

🌴 Visual Studio Code Version : [ 1.44.2 ]
🌴 Code Settings Sync Version : [ 3.4.3 ]
🌴 Operating System : [ Win 7 ]
🌴 Occurs On: [Login with Github ]
🌴 Proxy Enabled: [ No ]
🌴 Gist Id: [ a0c1009bcc22ca80f04b2e70e1e7d99d]
💪 Expected behavior
login my github and sync my setting

Duplicate of #1005


fix: fix dynamic field reading in walk

Codecov Report

Merging #6 (ece5dd6) into main (4fd91a5) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main        #6   +/-   ##
=========================================
  Coverage   100.00%   100.00%
=========================================
  Files            2         2
  Lines          349       356    +7
  Branches        63        63
=========================================
+ Hits           349       356    +7

Impacted Files
Coverage Δ

src/index.tsx
100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa01aa0...ece5dd6. Read the comment docs.


Will not build with Dart 2.1.0

I've just updated to the newest flutter release (dev). Now I get this:
The current Dart SDK version is 2.1.0-dev.0.0.flutter-be6309690f.

Because ... depends on audioplayers >=0.2.0 which requires SDK version >=1.8.0 <2.0.0 or >=2.0.0-dev.50.0 <2.0.0, version solving failed.
pub get failed (1)

Merged! Thanks, guys!

@luanpotter can you bump the version please? :)

@sroddy 0.7.5 has the fix ;)


Add atom's static/variables into include path

Four files there should be automatically detected when imported.

You can add the path it to the include paths in the linter-less settings.


Update outdated comments

What this PR does / why we need it:
The comment is outdated and updated
Which issue(s) this PR fixes

Fixes #
Please leave this checklist in the PR comment so that maintainers can ensure a good PR.

/assign @carolynvs

/retest

/assign @carolynvs

/ok-to-test
/lgtm
/approve


[WIP] shorten the names of the workers for deployments

Background
OpenShift only allows worker names to be up to 63 characters long.
To ensure the worker name is less than 63, we have a test. ref
The name is something like:
  def worker_deployment_name
    #{server_id}-#{abbreviated_class_name}-#{ems.id}
  end

Issue
In my local test setup:

the server id is 8 characters long
the abbreviated_class_name is up to 61 characters long (with many in the 50's)
the ems_id can be another 8 characters (but typically under 4) -- it is blank on my examples
results is much longer than the allowed 63.

Solution
Lets introduce some rules to shorten the abbreviated_class_name.
We already remove some phrases, so removing a few more phrases is the next logical step.
These rules are currently in worker_deployment_name, but they could probably move to abbreviated_class_name. Also, providers could introduce a provider specific rule, especially some of the ibm cloud ones that are quite long.
Values

old
len
old
new minimal_class_name

33
33
Amazon::AgentCoordinator
Amazon::AgentCoordinator

44
29
Amazon::CloudManager::EventCatcher
Amazon::Cloud::Events

47
29
Amazon::CloudManager::MetricsCollector
Amazon::Cloud::Metrics

38
30
Amazon::CloudManager::Refresh
Amazon::Cloud::Refresh

48
33
Amazon::ContainerManager::EventCatcher
Amazon::Container::Events

42
34
Amazon::ContainerManager::Refresh
Amazon::Container::Refresh

43
27
Amazon::StorageManager::S3::Refresh
Amazon::S3::Refresh

56
41
AnsibleTower::AutomationManager::EventCatcher
AnsibleTower::Automation::Events

50
42
AnsibleTower::AutomationManager::Refresh
AnsibleTower::Automation::Refresh

41
25
Autosde::StorageManager::Refresh
Autosde::Refresh

43
28
Azure::CloudManager::EventCatcher
Azure::Cloud::Events

46
28
Azure::CloudManager::MetricsCollector
Azure::Cloud::Metrics

37
29
Azure::CloudManager::Refresh
Azure::Cloud::Refresh

47
32
Azure::ContainerManager::EventCatcher
Azure::Container::Events

41
33
Azure::ContainerManager::Refresh
Azure::Container::Refresh

49
34
AzureStack::CloudManager::EventCatcher
AzureStack::Cloud::Events

43
35
AzureStack::CloudManager::Refresh
AzureStack::Cloud::Refresh

45
37
AzureStack::NetworkManager::Refresh
AzureStack::Network::Refresh

63
42
CiscoIntersight::PhysicalInfraManager::EventCatcher
CiscoIntersight::Physical::Events

66
42
CiscoIntersight::PhysicalInfraManager::MetricsCollector
CiscoIntersight::Physical::Metrics

57
43
CiscoIntersight::PhysicalInfraManager::Refresh
CiscoIntersight::Physical::Refresh

47
39
Foreman::ConfigurationManager::Refresh
Foreman::Configuration::Refresh

44
29
Google::CloudManager::EventCatcher
Google::Cloud::Events

47
29
Google::CloudManager::MetricsCollector
Google::Cloud::Metrics

38
30
Google::CloudManager::Refresh
Google::Cloud::Refresh

48
33
Google::ContainerManager::EventCatcher
Google::Container::Events

42
34
Google::ContainerManager::Refresh
Google::Container::Refresh

45
30
IbmCic::CloudManager::EventCatcher
IbmCic::Cloud::Events

48
30
IbmCic::CloudManager::MetricsCollector
IbmCic::Cloud::Metrics

39
31
IbmCic::CloudManager::Refresh
IbmCic::Cloud::Refresh

47
32
IbmCic::NetworkManager::EventCatcher
IbmCic::Network::Events

62
31
IbmCic::StorageManager::CinderManager::EventCatcher
IbmCic::Cinder::Events

51
36
IbmCloud::ContainerManager::EventCatcher
IbmCloud::Container::Events

54
36
IbmCloud::ContainerManager::MetricsCollector
IbmCloud::Container::Metrics

45
37
IbmCloud::ContainerManager::Refresh
IbmCloud::Container::Refresh

58
34
IbmCloud::ObjectStorage::StorageManager::Refresh
IbmCloud::Object::Refresh

69
41
IbmCloud::PowerVirtualServers::CloudManager::EventCatcher
IbmCloud::PowerVS::Cloud::Events

63
42
IbmCloud::PowerVirtualServers::CloudManager::Refresh
IbmCloud::PowerVS::Cloud::Refresh

51
36
IbmCloud::VPC::CloudManager::EventCatcher
IbmCloud::VPC::Cloud::Events

54
36
IbmCloud::VPC::CloudManager::MetricsCollector
IbmCloud::VPC::Cloud::Metrics

45
37
IbmCloud::VPC::CloudManager::Refresh
IbmCloud::VPC::Cloud::Refresh

51
36
IbmPowerHmc::InfraManager::EventCatcher
IbmPowerHmc::Infra::Events

54
36
IbmPowerHmc::InfraManager::MetricsCollector
IbmPowerHmc::Infra::Metrics

45
37
IbmPowerHmc::InfraManager::Refresh
IbmPowerHmc::Infra::Refresh

50
35
IbmPowerVc::CloudManager::EventCatcher
IbmPowerVc::Cloud::Events

53
35
IbmPowerVc::CloudManager::MetricsCollector
IbmPowerVc::Cloud::Metrics

44
36
IbmPowerVc::CloudManager::Refresh
IbmPowerVc::Cloud::Refresh

52
37
IbmPowerVc::NetworkManager::EventCatcher
IbmPowerVc::Network::Events

67
36
IbmPowerVc::StorageManager::CinderManager::EventCatcher
IbmPowerVc::Cinder::Events

53
45
IbmTerraform::ConfigurationManager::Refresh
IbmTerraform::Configuration::Refresh

52
37
Kubernetes::ContainerManager::EventCatcher
Kubernetes::Container::Events

55
37
Kubernetes::ContainerManager::MetricsCollector
Kubernetes::Container::Metrics

46
38
Kubernetes::ContainerManager::Refresh
Kubernetes::Container::Refresh

53
38
Kubernetes::MonitoringManager::EventCatcher
Kubernetes::Monitoring::Events

40
32
Kubevirt::InfraManager::Refresh
Kubevirt::Infra::Refresh

53
32
Lenovo::PhysicalInfraManager::EventCatcher
Lenovo::Physical::Events

47
33
Lenovo::PhysicalInfraManager::Refresh
Lenovo::Physical::Refresh

41
33
Microsoft::InfraManager::Refresh
Microsoft::Infra::Refresh

38
30
Nsxt::NetworkManager::Refresh
Nsxt::Network::Refresh

45
30
Nuage::NetworkManager::EventCatcher
Nuage::Network::Events

39
31
Nuage::NetworkManager::Refresh
Nuage::Network::Refresh

51
36
Openshift::ContainerManager::EventCatcher
Openshift::Container::Events

54
36
Openshift::ContainerManager::MetricsCollector
Openshift::Container::Metrics

45
37
Openshift::ContainerManager::Refresh
Openshift::Container::Refresh

52
37
Openshift::MonitoringManager::EventCatcher
Openshift::Monitoring::Events

47
32
Openstack::CloudManager::EventCatcher
Openstack::Cloud::Events

50
32
Openstack::CloudManager::MetricsCollector
Openstack::Cloud::Metrics

41
33
Openstack::CloudManager::Refresh
Openstack::Cloud::Refresh

47
32
Openstack::InfraManager::EventCatcher
Openstack::Infra::Events

50
32
Openstack::InfraManager::MetricsCollector
Openstack::Infra::Metrics

41
33
Openstack::InfraManager::Refresh
Openstack::Infra::Refresh

49
34
Openstack::NetworkManager::EventCatcher
Openstack::Network::Events

64
33
Openstack::StorageManager::CinderManager::EventCatcher
Openstack::Cinder::Events

50
35
OracleCloud::CloudManager::EventCatcher
OracleCloud::Cloud::Events

53
35
OracleCloud::CloudManager::MetricsCollector
OracleCloud::Cloud::Metrics

44
36
OracleCloud::CloudManager::Refresh
OracleCloud::Cloud::Refresh

54
39
OracleCloud::ContainerManager::EventCatcher
OracleCloud::Container::Events

48
40
OracleCloud::ContainerManager::Refresh
OracleCloud::Container::Refresh

54
33
Redfish::PhysicalInfraManager::EventCatcher
Redfish::Physical::Events

48
34
Redfish::PhysicalInfraManager::Refresh
Redfish::Physical::Refresh

44
29
Redhat::InfraManager::EventCatcher
Redhat::Infra::Events

47
29
Redhat::InfraManager::MetricsCollector
Redhat::Infra::Metrics

38
30
Redhat::InfraManager::Refresh
Redhat::Infra::Refresh

46
31
Redhat::NetworkManager::EventCatcher
Redhat::Network::Events

40
32
Redhat::NetworkManager::Refresh
Redhat::Network::Refresh

44
29
Vmware::CloudManager::EventCatcher
Vmware::Cloud::Events

38
30
Vmware::CloudManager::Refresh
Vmware::Cloud::Refresh

48
33
Vmware::ContainerManager::EventCatcher
Vmware::Container::Events

42
34
Vmware::ContainerManager::Refresh
Vmware::Container::Refresh

44
29
Vmware::InfraManager::EventCatcher
Vmware::Infra::Events

47
29
Vmware::InfraManager::MetricsCollector
Vmware::Infra::Metrics

41
33
Vmware::InfraManager::Operations
Vmware::Infra::Operations

38
30
Vmware::InfraManager::Refresh
Vmware::Infra::Refresh

30
26
EmsMetricsProcessor
MetricsProcessor

22
22
EventHandler
EventHandler

16
16
Generic
Generic

17
17
Priority
Priority

23
23
RemoteConsole
RemoteConsole

18
18
Reporting
Reporting

17
17
Schedule
Schedule

20
20
SmartProxy
SmartProxy

11
11
Ui
Ui

20
20
WebService
WebService

WIP:

left debug code for before / after
left comments so others can play around with the code if desired

I thought we had an open issue where we were discussing some shortening ideas like this but I can't find it.  would like @jrafanie @agrare and @bdunne opinions, but in general I'm good with this.
One note - there is a trailing hyphen in some of the names in your chart...is that a typo?

Not sure about event-handler -> events, since events is also used for the event collectors.  Thinking we should keep that one as is.
Also I'm wondering if ems-metrics-processor should become metrics-processor

Openstack only allows worker names to be up to 63 characters long.

You mean openshift?

@agrare oracle-cloud-cloud is kind of weird.  Wondering if that can just be oracle instead of oracle_cloud.
Another thought I had is maybe instead of monkeying around with abbreviating the provider name, we use the ems_type, which is already a shortened form.

oracle-cloud-cloud is kind of weird. Wondering if that can just be oracle instead of oracle_cloud.

Yeah I agree, think this further supports this being more pluggable because we don't want .sub("OracleCloud", "Oracle") in ::MiqWorker

we use the ems_type, which is already a shortened form.

.ems_type is usually something like self.name.underscore (as you said OracleCloud is :oracle_cloud and worse, IbmCloud::PowerVirtualServers is :ibm_cloud_power_virtual_servers).  This could reduce the cloud-cloud duplication though e.g. OracleCloud::CloudManager => oracle_cloud-cloud and IbmCloud::PowerVirtualServers::CloudManager => ibm_cloud-power_virtual_servers-cloud each of these would become oracle-cloud and ibm_cloud_power_virtual_servers.  We could even change power's ems_type to ibm_cloud_powervs if we do go this route.

@fryguy Since EventCatcher is uses often and in longer names, reducing that buys us a lot.
On the other hand, EventHandler is only used once at the top level. leaving that alone is good.
The names 54 chars and longer were my target:
AnsibleTower::AutomationManager::EventCatcher
CiscoIntersight::PhysicalInfraManager::EventCatcher
CiscoIntersight::PhysicalInfraManager::MetricsCollector
IbmCloud::ContainerManager::MetricsCollector
IbmCloud::VPC::CloudManager::MetricsCollector
IbmPowerVc::StorageManager::CinderManager::EventCatcher
Kubernetes::ContainerManager::MetricsCollector
Openstack::StorageManager::CinderManager::EventCatcher
OracleCloud::ContainerManager::EventCatcher

Yeah :+1: for EventCatcher -> Event because that is a provider worker the EventHandler is only in core and so pretty short to begin with and would be good to differentiate from the catchers since it really does a different job.

Sorry, can we have a table of the worker_deployment_name before and after?  I can't tell if anything is changed for that method.  If it's changed, we'd likely need to ensure any documentation that mentions pod names are also updated.
I think you're dropping catcher but not sure what else is being changed.
I think the abbreviated_class_name will only change the process titles seen in top/ls and probably safe to change.  The worker_deployment_name might have other side effects we need to track.  For example, we index the list of "current" pods by pod name, which is determined by deployment name.  That list should refresh with each boot but there could be something else that expects the old deployment name.

yeah ems_type doesn't work, and I'm not sure what will happen if we change it since I think it gets written into a column.  Let's just not use it.
Instead, maybe we should introduce another method that knows how to present an abbreviated EMS name at the EMS level (as opposed to the worker level)?  I could see the default being what we do now, and the provider can override.  So, some possible examples:

OracleCloud::CloudManager can override with oracle_cloud
OracleCloud::ContainerManager can override with oke
IbmCloud::PowerVirtualServers::CloudManager can override with powervs

It is easier for me to read class names, and people seem to describe the changes using the class name.
I will change the table to show the deploy names before / after if that is easier for you to discuss. I'll leave the unchanged values in there so people can brainstorm on those
The goal is to get test passing locally on my machine.
And they fail because they are too long and would blow up on openshift.
(maybe they won't be deployed on openshift, so it is moot)

old deployment name
new deployment name

35r24369-amazon-agent-coordinator
35r24369-amazon-agent-coordinator

35r24369-amazon-cloud-manager-event-catcher-
35r24369-amazon-cloud-events-

35r24369-amazon-cloud-manager-metrics-collector
35r24369-amazon-cloud-metrics

35r24369-amazon-cloud-manager-refresh-
35r24369-amazon-cloud-refresh-

35r24369-amazon-container-manager-event-catcher-
35r24369-amazon-container-events-

35r24369-amazon-container-manager-refresh-
35r24369-amazon-container-refresh-

35r24369-amazon-storage-manager-s3-refresh-
35r24369-amazon-s3-refresh-

35r24369-ansible-tower-automation-manager-event-catcher-
35r24369-ansible-tower-automation-events-

35r24369-ansible-tower-automation-manager-refresh-
35r24369-ansible-tower-automation-refresh-

35r24369-autosde-storage-manager-refresh-
35r24369-autosde-refresh-

35r24369-azure-cloud-manager-event-catcher-
35r24369-azure-cloud-events-

35r24369-azure-cloud-manager-metrics-collector
35r24369-azure-cloud-metrics

35r24369-azure-cloud-manager-refresh-
35r24369-azure-cloud-refresh-

35r24369-azure-container-manager-event-catcher-
35r24369-azure-container-events-

35r24369-azure-container-manager-refresh-
35r24369-azure-container-refresh-

35r24369-azure-stack-cloud-manager-event-catcher-
35r24369-azure-stack-cloud-events-

35r24369-azure-stack-cloud-manager-refresh-
35r24369-azure-stack-cloud-refresh-

35r24369-azure-stack-network-manager-refresh-
35r24369-azure-stack-network-refresh-

35r24369-cisco-intersight-physical-infra-manager-event-catcher-
35r24369-cisco-intersight-physical-events-

35r24369-cisco-intersight-physical-infra-manager-metrics-collector
35r24369-cisco-intersight-physical-metrics

35r24369-cisco-intersight-physical-infra-manager-refresh-
35r24369-cisco-intersight-physical-refresh-

35r24369-foreman-configuration-manager-refresh-
35r24369-foreman-configuration-refresh-

35r24369-google-cloud-manager-event-catcher-
35r24369-google-cloud-events-

35r24369-google-cloud-manager-metrics-collector
35r24369-google-cloud-metrics

35r24369-google-cloud-manager-refresh-
35r24369-google-cloud-refresh-

35r24369-google-container-manager-event-catcher-
35r24369-google-container-events-

35r24369-google-container-manager-refresh-
35r24369-google-container-refresh-

35r24369-ibm-cic-cloud-manager-event-catcher-
35r24369-ibm-cic-cloud-events-

35r24369-ibm-cic-cloud-manager-metrics-collector
35r24369-ibm-cic-cloud-metrics

35r24369-ibm-cic-cloud-manager-refresh-
35r24369-ibm-cic-cloud-refresh-

35r24369-ibm-cic-network-manager-event-catcher-
35r24369-ibm-cic-network-events-

35r24369-ibm-cic-storage-manager-cinder-manager-event-catcher-
35r24369-ibm-cic-cinder-events-

35r24369-ibm-cloud-container-manager-event-catcher-
35r24369-ibm-cloud-container-events-

35r24369-ibm-cloud-container-manager-metrics-collector
35r24369-ibm-cloud-container-metrics

35r24369-ibm-cloud-container-manager-refresh-
35r24369-ibm-cloud-container-refresh-

35r24369-ibm-cloud-object-storage-storage-manager-refresh-
35r24369-ibm-cloud-object-refresh-

35r24369-ibm-cloud-power-virtual-servers-cloud-manager-event-catcher-
35r24369-ibm-cloud-power-vs-cloud-events-

35r24369-ibm-cloud-power-virtual-servers-cloud-manager-refresh-
35r24369-ibm-cloud-power-vs-cloud-refresh-

35r24369-ibm-cloud-vpc-cloud-manager-event-catcher-
35r24369-ibm-cloud-vpc-cloud-events-

35r24369-ibm-cloud-vpc-cloud-manager-metrics-collector
35r24369-ibm-cloud-vpc-cloud-metrics

35r24369-ibm-cloud-vpc-cloud-manager-refresh-
35r24369-ibm-cloud-vpc-cloud-refresh-

35r24369-ibm-power-hmc-infra-manager-event-catcher-
35r24369-ibm-power-hmc-infra-events-

35r24369-ibm-power-hmc-infra-manager-metrics-collector
35r24369-ibm-power-hmc-infra-metrics

35r24369-ibm-power-hmc-infra-manager-refresh-
35r24369-ibm-power-hmc-infra-refresh-

35r24369-ibm-power-vc-cloud-manager-event-catcher-
35r24369-ibm-power-vc-cloud-events-

35r24369-ibm-power-vc-cloud-manager-metrics-collector
35r24369-ibm-power-vc-cloud-metrics

35r24369-ibm-power-vc-cloud-manager-refresh-
35r24369-ibm-power-vc-cloud-refresh-

35r24369-ibm-power-vc-network-manager-event-catcher-
35r24369-ibm-power-vc-network-events-

35r24369-ibm-power-vc-storage-manager-cinder-manager-event-catcher-
35r24369-ibm-power-vc-cinder-events-

35r24369-ibm-terraform-configuration-manager-refresh-
35r24369-ibm-terraform-configuration-refresh-

35r24369-kubernetes-container-manager-event-catcher-
35r24369-kubernetes-container-events-

35r24369-kubernetes-container-manager-metrics-collector
35r24369-kubernetes-container-metrics

35r24369-kubernetes-container-manager-refresh-
35r24369-kubernetes-container-refresh-

35r24369-kubernetes-monitoring-manager-event-catcher-
35r24369-kubernetes-monitoring-events-

35r24369-kubevirt-infra-manager-refresh-
35r24369-kubevirt-infra-refresh-

35r24369-lenovo-physical-infra-manager-event-catcher-
35r24369-lenovo-physical-events-

35r24369-lenovo-physical-infra-manager-refresh-
35r24369-lenovo-physical-refresh-

35r24369-microsoft-infra-manager-refresh-
35r24369-microsoft-infra-refresh-

35r24369-nsxt-network-manager-refresh-
35r24369-nsxt-network-refresh-

35r24369-nuage-network-manager-event-catcher-
35r24369-nuage-network-events-

35r24369-nuage-network-manager-refresh-
35r24369-nuage-network-refresh-

35r24369-openshift-container-manager-event-catcher-
35r24369-openshift-container-events-

35r24369-openshift-container-manager-metrics-collector
35r24369-openshift-container-metrics

35r24369-openshift-container-manager-refresh-
35r24369-openshift-container-refresh-

35r24369-openshift-monitoring-manager-event-catcher-
35r24369-openshift-monitoring-events-

35r24369-openstack-cloud-manager-event-catcher-
35r24369-openstack-cloud-events-

35r24369-openstack-cloud-manager-metrics-collector
35r24369-openstack-cloud-metrics

35r24369-openstack-cloud-manager-refresh-
35r24369-openstack-cloud-refresh-

35r24369-openstack-infra-manager-event-catcher-
35r24369-openstack-infra-events-

35r24369-openstack-infra-manager-metrics-collector
35r24369-openstack-infra-metrics

35r24369-openstack-infra-manager-refresh-
35r24369-openstack-infra-refresh-

35r24369-openstack-network-manager-event-catcher-
35r24369-openstack-network-events-

35r24369-openstack-storage-manager-cinder-manager-event-catcher-
35r24369-openstack-cinder-events-

35r24369-oracle-cloud-cloud-manager-event-catcher-
35r24369-oracle-cloud-cloud-events-

35r24369-oracle-cloud-cloud-manager-metrics-collector
35r24369-oracle-cloud-cloud-metrics

35r24369-oracle-cloud-cloud-manager-refresh-
35r24369-oracle-cloud-cloud-refresh-

35r24369-oracle-cloud-container-manager-event-catcher-
35r24369-oracle-cloud-container-events-

35r24369-oracle-cloud-container-manager-refresh-
35r24369-oracle-cloud-container-refresh-

35r24369-redfish-physical-infra-manager-event-catcher-
35r24369-redfish-physical-events-

35r24369-redfish-physical-infra-manager-refresh-
35r24369-redfish-physical-refresh-

35r24369-redhat-infra-manager-event-catcher-
35r24369-redhat-infra-events-

35r24369-redhat-infra-manager-metrics-collector
35r24369-redhat-infra-metrics

35r24369-redhat-infra-manager-refresh-
35r24369-redhat-infra-refresh-

35r24369-redhat-network-manager-event-catcher-
35r24369-redhat-network-events-

35r24369-redhat-network-manager-refresh-
35r24369-redhat-network-refresh-

35r24369-vmware-cloud-manager-event-catcher-
35r24369-vmware-cloud-events-

35r24369-vmware-cloud-manager-refresh-
35r24369-vmware-cloud-refresh-

35r24369-vmware-container-manager-event-catcher-
35r24369-vmware-container-events-

35r24369-vmware-container-manager-refresh-
35r24369-vmware-container-refresh-

35r24369-vmware-infra-manager-event-catcher-
35r24369-vmware-infra-events-

35r24369-vmware-infra-manager-metrics-collector
35r24369-vmware-infra-metrics

35r24369-vmware-infra-manager-operations-
35r24369-vmware-infra-operations-

35r24369-vmware-infra-manager-refresh-
35r24369-vmware-infra-refresh-

35r24369-ems-metrics-processor
35r24369-metrics-processor

35r24369-event-handler
35r24369-event-handler

35r24369-generic
35r24369-generic

35r24369-priority
35r24369-priority

35r24369-remote-console
35r24369-remote-console

35r24369-reporting
35r24369-reporting

35r24369-schedule
35r24369-schedule

35r24369-smart-proxy
35r24369-smart-proxy

35r24369-ui
35r24369-ui

35r24369-web-service
35r24369-web-service

old deployment name	new deployment name
✂
35r24369-openstack-cloud-manager-event-catcher-	35r24369-openstack-cloud-events-
35r24369-openstack-cloud-manager-metrics-collector	35r24369-openstack-cloud-metrics
35r24369-openstack-cloud-manager-refresh-	35r24369-openstack-cloud-refresh-
35r24369-openstack-infra-manager-event-catcher-	35r24369-openstack-infra-events-
35r24369-openstack-infra-manager-metrics-collector	35r24369-openstack-infra-metrics
35r24369-openstack-infra-manager-refresh-	35r24369-openstack-infra-refresh-
35r24369-openstack-network-manager-event-catcher-	35r24369-openstack-network-events-
35r24369-openstack-storage-manager-cinder-manager-event-catcher-	35r24369-openstack-cinder-events-
35r24369-oracle-cloud-cloud-manager-event-catcher-	35r24369-oracle-cloud-cloud-events-
35r24369-oracle-cloud-cloud-manager-metrics-collector	35r24369-oracle-cloud-cloud-metrics
35r24369-oracle-cloud-cloud-manager-refresh-	35r24369-oracle-cloud-cloud-refresh-
35r24369-oracle-cloud-container-manager-event-catcher-	35r24369-oracle-cloud-container-events-
35r24369-oracle-cloud-container-manager-refresh-	35r24369-oracle-cloud-container-refresh-
35r24369-redfish-physical-infra-manager-event-catcher-	35r24369-redfish-physical-events-
35r24369-redfish-physical-infra-manager-refresh-	35r24369-redfish-physical-refresh-
35r24369-redhat-infra-manager-event-catcher-	35r24369-redhat-infra-events-
35r24369-redhat-infra-manager-metrics-collector	35r24369-redhat-infra-metrics
35r24369-redhat-infra-manager-refresh-	35r24369-redhat-infra-refresh-
35r24369-redhat-network-manager-event-catcher-	35r24369-redhat-network-events-
35r24369-redhat-network-manager-refresh-	35r24369-redhat-network-refresh-
35r24369-vmware-cloud-manager-event-catcher-	35r24369-vmware-cloud-events-
35r24369-vmware-cloud-manager-refresh-	35r24369-vmware-cloud-refresh-
35r24369-vmware-container-manager-event-catcher-	35r24369-vmware-container-events-
35r24369-vmware-container-manager-refresh-	35r24369-vmware-container-refresh-
35r24369-vmware-infra-manager-event-catcher-	35r24369-vmware-infra-events-
35r24369-vmware-infra-manager-metrics-collector	35r24369-vmware-infra-metrics
35r24369-vmware-infra-manager-operations-	35r24369-vmware-infra-operations-
35r24369-vmware-infra-manager-refresh-	35r24369-vmware-infra-refresh-
35r24369-ems-metrics-processor	35r24369-metrics-processor
35r24369-event-handler	35r24369-event-handler
35r24369-generic	35r24369-generic
35r24369-priority	35r24369-priority
35r24369-remote-console	35r24369-remote-console
35r24369-reporting	35r24369-reporting
35r24369-schedule	35r24369-schedule
35r24369-smart-proxy	35r24369-smart-proxy
35r24369-ui	35r24369-ui
35r24369-web-service	35r24369-web-service

I don't think the old name had the compressed id prefix... but anyway, I don't really know if changing the deployment name will have side effects so I think we'd need to test podified and appliances plus upgrades.  Do we really care about the deployment name?  If not, we can just truncate it if it's too long.

I don't think the old name had the compressed id prefix

Old name definitely had compressed id, however in podified we almost always have region 0, and the compressed id code leaves out the 0r for region 0.

@kbrock Your "before" still shows a trailing -.  Are there really trailing - in those or is that a typo?

Ah interesting - then IMO, some fake number should in the examples above similar to how the region/server is just some number.

I thought the unit names were "dynamic" and that the orchestrator would delete ones it doesn't recognize?

We moved these to be statically defined as part of the non-root work

If they are static and deployed as part of the RPMs, then an RPM upgrade should remove the old and add the new automatically.  Only time we run into problems is, I think, dynamic ones right?

If they are static and deployed as part of the RPMs, then an RPM upgrade should remove the old and add the new automatically.

This is exactly what I was saying :laughing:  just didn't want us to forget about it if we do this.

The specs set the ems_id. Not sure why they do not show up in the puts/values.
would prefer not changing the systemd value. Would also prefer not having 2 different methods to derive the name.

These names sure feel like Java namespaces

Think my biggest problem I had with this is the fact that the systemd service file name changed. (I mean, it would be nice to shorten those names, but I don't want to deal with enabling and disabling them across rpm upgrades)
Keeping these rules in worker_deployment_name seems like a good way to move forward.

I am happy with the code at this point.
-Leaving as a WIP because I don't believe the discussion was done

And the systemd deployment names are based upon minimal_class_name and not based upon the method we changed here (i.e.: abbreviated_class_name), so the systemd files should not be changed.

Would have liked to get these changes into systemd. But changing the service file names had me screwed up so I went the additinal method route.
@bdunne do you think changing the deployment name will alter existing systems after an upgrade?


[$1000] Correct chat is not opened by tapping chat link

If you haven’t already, check out our contributing guidelines for onboarding and email<EMAIL_ADDRESS>to request to join our Slack channel!

This issue was found when retesting #19914
Action Performed:

Using Android m/Web, login to NewDot with account A and open any DM. Copy the report url from the address bar (like https://staging.new.expensify.com/r/1242891003534332
)
Paste it in any external app (Slack or Telegram)
Log in with same account (account A) in android app
In the android app, open any other chat (not the one you opened in step 1)
Go back to the external app and tap link you pasted in external app

Expected Result:
Chat from step 1 should be opened without any visual issues.
Actual Result:
Chat from step 4 stays on the screen
Workaround:
Can the user still use Expensify without this being fixed? Have you informed them of the workaround?
Platforms:

Which of our officially supported platforms is this issue occurring on?

[x] Android / native
[ ] Android / Chrome
[ ] iOS / native
[ ] iOS / Safari
[ ] MacOS / Chrome / Safari
[ ] MacOS / Desktop

Version Number: 1.3.26-3
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
https://github.com/Expensify/App/assets/43996225/9c212204-3947-4e65-bc38-353800806187
Expensify/Expensify Issue URL:
Issue reported by: applause internal team
Slack conversation:
View all open jobs on GitHub
Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b971875418ce264c
Upwork Job ID:<PHONE_NUMBER>229225472
Last Price Increase: 2023-06-21

Proposal
Please re-state the problem that we are trying to solve in this issue.
Correct chat is not opened by tapping chat link.
What is the root cause of that problem?
This is what happens now if the app is opened in background and we try to open a chat from a deep link:

In Expensify.js we call Linking.addEventListener('url', (state) => Report.openReportFromDeepLink(state.url, isAuthenticated));;
In Report.js we call Navigation.navigate(ROUTES.getReportRoute(reportID));;
In Navigation.js we call linkTo(navigationRef.current, route, type);;
In linkTo.js we call:

if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
            action.type = 'PUSH';

Now, here is the problem
Both getTopmostReportId(root.getState()) and getTopmostReportId(state) return the same report id, so we never set action.type = 'PUSH', so we never navigate to the new report.
Why does this happen?
getTopmostReportId(state) returns the reportId from the URL, which is the new report ID where we need to navigate to. But, when coming from a deep link, the NavigationContainer has the new state inserted automatically as this is it's default behavior(one may check this as onStateChange callback is automatically triggered when the app gets reopened from the deep link), so getTopmostReportId(root.getState()) returns the new report id, even if we did not navigate there yet.
What changes do you think we should make in order to solve the problem?

As everything starts from openReportFromDeepLink, we may pass an optional parameter set to false by default, in order to let linkTo know when we come from a deep link.
We know getTopmostReportId(root.getState()) will return the same reportId when coming from a deep link, so we need another way to get the last visible reportId. I have some options in mind such as:
2.1. Either, when onStateChange gets called for the NavigationContainer, in updateSavedNavigationStateAndLogRoute we may also save the previousState;
2.2. Or, add an event listener for when app goes to background on native, same as we already have the one from when it comes back active. Inside it, save lastReportId which is going to be the current opened report id if any, or null/undefined otherwise.
2.3 Or, I may think of other ways to get the topmost report id inside linkTo if previous 2 are not the best, let me know.
Now, inside linkTo, we may update the if, such that when coming from a deep link, will use the other way to get the topmost report id instead of getTopmostReportId(root.getState()).

What alternative solutions did you explore?
I. If we don't want to add and pass a new parameter when coming from a deep link, we may completely move from using getTopmostReportId(root.getState()) to always saving a report id in a context when opening a report, and resetting it when closing one, which will be used here. linkTo.js would look like:
const topmostReportId = the one from where we saved it;
if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && topmostReportId !== getTopmostReportId(state)) {
            action.type = 'PUSH';

II. If we don't want to save the topmost report id, we may also just pass when coming from a deep link, and set action.type = 'REPLACE', thus we'll replace the current report with the one from the deep link every time, but this will mess the back navigation as the previous screen was replaced with this one.

Proposal
Please re-state the problem that we are trying to solve in this issue.
Unable to open a different chat by deeplink
What is the root cause of that problem?
Currently, we have a deep link handler specifically for report screen introduced in this PR
https://github.com/Expensify/App/blob/862955c2f15bce3c68d87c549b7af0e27a0eb526/src/Expensify.js#L163-L167
Why we have it for report screen only? The best guess I have is, before the navigation reboot, we use a drawer navigator for LHN and report screen, so we need a custom handler for report screen that will handle whether to close or open the drawer. Below is the screenshot of the old Navigation.navigate. If it's a drawer route, which is true for report screen, it will do a custom action.

Now, we have completely moved to stack navigator and the navigate implementation is updated. When we navigate to a report screen, it will compare the current report screen id and the report id we want to navigate (both id is taken from navigation state). If it's different, we want to push it to the stack. This works well when we just doing a simple navigation, but not with deeplink.
https://github.com/Expensify/App/blob/862955c2f15bce3c68d87c549b7af0e27a0eb526/src/libs/Navigation/linkTo.js#L26-L30
When we are doing a deeplink, rn-navigation will navigate to the target report id, even without the custom deeplink handler we have. So, when we deeplink to report B, the root navigation state is already updated with the report B id, so when the custom handler runs, both current report screen id and target report id is the same, thus the navigation action type stays as NAVIGATE instead of PUSH.
NAVIGATE action won't push the new screen if it's already exist in the stack, that's why I believe we have this commit that will change the type to PUSH if the report id is different.
What changes do you think we should make in order to solve the problem?

As the intention is to push the new report screen to stack if the report ID is different, I think the better way to handle it is by adding getId props to report screen here:
https://github.com/Expensify/App/blob/862955c2f15bce3c68d87c549b7af0e27a0eb526/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js#L18-L30
getId will return report id.
getId={({params}) => params.reportID}

This will always push the new report screen if the report ID is not in the stack yet. Also, we don't need this logic anymore.
https://github.com/Expensify/App/blob/862955c2f15bce3c68d87c549b7af0e27a0eb526/src/libs/Navigation/linkTo.js#L28-L31
Or is there any clear reason we are doing it that way?

@rushatgabhane There are a few proposals for you to review! 🤗

@rushatgabhane It's been 3 days since your last message, can you please prioritize taking a look here? Thank you!!

@rushatgabhane

@DanutGavrus your https://github.com/Expensify/App/issues/20624#issuecomment-1590023343 is well explained too. But I like @bernhardoj's solution more because it's fixing the root cause and it's simple.

I agree! That was the ID I was looking for, but missed 😁

Before continue with my proposal, can we have a review from the SWM team that did the navigation reboot?

Or is there any clear reason we are doing it that way?

I think we also need the answer of this. That would be great if we can have the confirmation from them.

@rushatgabhane I see on other navigation-related issues (example), sometimes we tag the SWM team to have an additional review of the proposal, so I think we can do the same here?

i reached out to SWM here - https://expensify.slack.com/archives/C04878MDF34/p1687886035526329
i asked if someone had availability to review the proposal. if so, they will comment on this issue 👍

@WoLewicki yes. Either by deeplink or navigating normally to the same report won't push the exact same report to the stack.

The snippet is on my proposal, but here is the diff to be clearer:

diff
diff --git a/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js b/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js
index 64eadcbe06..2c57e42771 100644
--- a/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js
+++ b/src/libs/Navigation/AppNavigator/Navigators/CentralPaneNavigator.js
@@ -27,6 +27,7 @@ function CentralPaneNavigator() {
                         cardStyle: styles.cardStyleNavigator,
                     }}
                     component={ReportScreenWrapper}
+                    getId={({params}) => params.reportID}
                 />
             </Stack.Navigator>
         </FreezeWrapper>
diff --git a/src/libs/Navigation/linkTo.js b/src/libs/Navigation/linkTo.js
index 1d219c450e..602e0a0642 100644
--- a/src/libs/Navigation/linkTo.js
+++ b/src/libs/Navigation/linkTo.js
@@ -25,13 +25,9 @@ export default function linkTo(navigation, path, type) {

     // If action type is different than NAVIGATE we can't change it to the PUSH safely
     if (action.type === 'NAVIGATE') {
-        // If this action is navigating to the report screen and the top most navigator is different from the one we want to navigate - PUSH
-        if (action.payload.name === NAVIGATORS.CENTRAL_PANE_NAVIGATOR && getTopmostReportId(root.getState()) !== getTopmostReportId(state)) {
-            action.type = 'PUSH';
-
-            // If this action is navigating to the RightModalNavigator and the last route on the root navigator is also RightModalNavigator
-            // then we want to replace the current RHP state with new one
-        } else if (type === 'UP') {
+        // If this action is navigating to the RightModalNavigator and the last route on the root navigator is also RightModalNavigator
+        // then we want to replace the current RHP state with new one
+        if (type === 'UP') {
             action.type = 'REPLACE';

             // If this action is navigating to the RightModalNavigator and the last route on the root navigator is not RightModalNavigator then push

Hmm looking at https://reactnavigation.org/docs/screen/#getid, what about deeplinking to a report that is already somewhere deeper in the stack? Won't it navigate back then? We want the new reports to be always pushed on top, even if we have already been on that report earlier.

We would probably have to add some unique id to each of those ids and then cut it when using in the component, but it should be rather easy. Or am I missing something @bernhardoj ?

Won't it navigate back then?

Yes.

We want the new reports to be always pushed on top, even if we have already been on that report earlier.

If this is the case, then my proposal won't work as expected.

We would probably have to add some unique id to each of those ids

getId only has params that contains reportID only. I actually have no idea what kind of unique ID we can append to reportID 😅.

@bernhardoj you can change the getId method to append some suffix to the reportID, e.g. the index of the screen you are at now so it is unique. Is it ok?

@WoLewicki

the index of the screen you are at now

I'm assuming we want the screen index of the report screen, which means the logic will be similar to getTopmostReportId. I tried to append it with getTopmostReportId but it doesn't work. getTopmostReportId will always return the report ID we "want" to navigate. I think this would be the same with index because route state is already updated before getId is called.

shouldn't it be applied to the RootStack screen with name={NAVIGATORS.CENTRAL_PANE_NAVIGATOR}

I just realized that navigating to the report will push CENTRAL_PANE_NAVIGATOR to the stack. I thought we would only have one CENTRAL_PANE_NAVIGATOR and a new report will be pushed to CENTRAL_PANE_NAVIGATOR. But adding the getId to name={SCREENS.REPORT} somehow works (I guess it checks the screen name?).

What do you mean by works? It still pushes it without setting it to PUSH in linkTo.js ?

Hmm it seems weird, but maybe it is how react-navigation works under the hood. Still, we need to handle the case of trying to navigate to the same report via deeplink.

@WoLewicki and @bernhardoj are working this out and will potentially review other proposals.

@rushatgabhane - will you review @hungvu193's comment here and provide an update on how you think we should move forward? thanks!

Issue not reproducible during KI retests. (First week)

I tried to reproduce it again. It does work if you are in the LHN menu, but still I can reproduce it if I have another chat opened.

Melvin got confused with the assignment haha

If @hungvu193 solution does not destroy the navigation history (the back buttons lead to correct pages) and also browser history (if it is reproducible on web, then clicking browser back button should also lead to correct page), then I think it can be the best option.

@WoLewicki @allroundexperts
From my Proposal on June 14th:

II. If we don't want to save the topmost report id, we may also just pass when coming from a deep link, and set action.type = 'REPLACE', thus we'll replace the current report with the one from the deep link every time, but this will mess the back navigation as the previous screen was replaced with this one.

This is what I meant with "we may also just pass when coming from a deep link"(parameter for the method) and with "set action.type"(implicit effect):
https://github.com/Expensify/App/blob/a23d1614c0e60f99dd51cf6f4fd08f65248203d1/src/libs/Navigation/linkTo.js#L34-L35
Then, isn't @hungvu193 solution a dupe?
Also:

If @hungvu193 solution does not destroy the navigation history (the back buttons lead to correct pages) and also browser history (if it is reproducible on web, then clicking browser back button should also lead to correct page).

The only thing which messes with the back navigation is the previous report which won't be there when going back, as it was replaced with the opened one as I've said back then:

[...] but this will mess the back navigation as the previous screen was replaced with this one.

What this means is that if I'm on a chat with friend A and then click on a link to a chat with friend B, chat A will get replaced with chat B, so pressing back will get us 1 screen before chat with friend A.

I think we can go with the replace solution since deeplinking to an app from external source can be seen as creating the new navigation flow in it.

@allroundexperts @WoLewicki
Not sure if this would work & actually be a better alternative, but thought to point it out. What if:

When app goes in background in native, save lastVisibleReportID in Onyx if it leaves from an opened report, null/undefined otherwise. We already have an app become visible callback implemented, and the app went to background callback will be very similar;
Then, instead of using getTopmostReportId(root.getState()) !== getTopmostReportId(state), we'll compare getTopmostReportId with the saved lastVisibleReportID?

Sounds good! Thank you everyone!

@allroundexperts
PR ready for review: PR22740

@allroundexperts
Yes, I'll look into this tomorrow!

I have some PRs in progress which I need to take care of, I'll come to this right after!

updated amounts!
@allroundexperts - i'll let you clarify the other details that @DanutGavrus is asking about!

@maddylewis

updated amounts!

The Upwork job seems to have the same amount.
@bernhardoj

react-navigation linking has a subscribe function to override the default deep link handler by react-navigation. It may help.

Couldn't figure it out with that approach.
@bernhardoj @allroundexperts @WoLewicki
I think I've eventually found a fix. This is our current openReportFromDeepLink implementation:
https://github.com/Expensify/App/blob/3d22f591054e5ace31afa58d965e382768e309d4/src/libs/actions/Report.js#L1694-L1722
And this is a potential fix:
function openReportFromDeepLink(url, isAuthenticated) {
  const activeRoute = Navigation.getActiveRoute();
  const {reportID: activeReportID} = ROUTES.parseReportRouteParams(activeRoute);
  const route = ReportUtils.getRouteFromLink(url);
  const reportID = ReportUtils.getReportIDFromLink(url);

  [...]

    SidebarUtils.isSidebarLoadedReady().then(() => {
      // Navigate to the new report only if one is already opened. Otherwise, the NavigationRoot navigates automatically.
      if (activeReportID && reportID && activeReportID != reportID) {
          Navigation.navigate(ROUTES.getReportRoute(reportID), 'UP');
      }

Deeplink opening a report from any screen which is not a report(Home, Settings Page, etc) does not open that report twice(as in the new issue) anymore;
Deeplink opening the same report which we are on does not replace it anymore.

[OPTIONAL CHANGE] On a side note, I think this may be extended as to remove 'UP' and pass a new activeReportID optional parameter to the navigate method and then to the linkTo method. Inside it, if an activeReportID was passed, we compare activeReportID != getTopmostReportId(state) where state = getStateFromPath(path), if not, we keep getTopmostReportId(root.getState()) !== getTopmostReportId(state). This would fix:

[OPTIONAL FIX] Deeplink opening a report replaces the actual one and back navigation does not bring us to previous report(The small problem we agreed may remain with that solution).

Let me know your thoughts about this! 👀

Payments:

[x] no reporting bonus
[ ] @allroundexperts ($1500 - paid via NewDot)
[ ] @DanutGavrus ($1500 - paid via Upwork)

requested refund of $1k from @DanutGavrus in Upwork (could not request refund for $1500 since the total invoice amount was $1k.

@maddylewis $1k refunded!

@WoLewicki Did this issue exist before or does any new navigation change caused this? Also, do you have any idea about the direction in which we should look for a solution here?

I believe the shouldn't be any other new changes that could have caused it, so probably it was an untested flow. I haven't got too much context of it since I did not take part in implementing the deeplinking support. What about the solution proposed in the issue: https://github.com/Expensify/App/issues/23441#issuecomment-1656725230 ? Can you see any drawbacks of it @DanutGavrus ?

Sorry, I missed this. I even checked the videos from the original PR and it seems it was working just fine back the, so something was introduced in between. Does this happen only on iOS? So I can test it as well.

@pecanoro It happens on Android too.

I can't reproduce it on Android on my phone, it's working well.

@maddylewis Are you able to reproduce the potential regression on your iOS phone? It's working well for me on 1.3.50-2

i am OOO until ~Aug 14. i can try to repro once i return or we can re-assign the bug label to have someone else try!

I think it is a regression actually - https://github.com/Expensify/App/issues/23441#issuecomment-1676360013

@abdulrahuman5196  Got it! It seems a contributor on the other issue found the root cause and a better solution for this issue and the regression. Are we having them fix it? If they confirmed the regression, we have to cut the payment for this one in half.

@abdulrahuman5196 If the other contributor said in the alternative Proposal that removing only the change from that PR does not cause the original issue anymore, can't it be that something has changed meanwhile(which fixed the original Issue under the hood and created the new one), thus not being a regression? 🤷

The PR still caused a regression since once the PR is reverted, the new bug disappears. However, if you can find me the PR that introduced those changes that might have broken your PR, I will reconsider it. So far that has been a theory but we have yet to find such PR.

@maddylewis It's all yours for payment now!

Payments:

[x] no reporting bonus
[ ] @allroundexperts ($1500 - paid via NewDot)
[ ] @DanutGavrus ($1000 - paid via Upwork - I originally paid $1500 and requested a $1k refund due to the regression, so @DanutGavrus is owed $1k at this point)

@pecanoro

once the PR is reverted, the new bug disappears

For some reason, same does the old bug.

So far that has been a theory but we have yet to find such PR.

Sorry, I wanted to try that sooner, but I was not able to run the app on Android for some time until now. So, I've gathered these facts:

The old issue was that: If chat A is opened, chat B won't get opened at all by deeplinking;
The new issue is that: If no chat is opened, chat B gets opened twice;
Reverting the PR fixes both old & new issues.

These facts give the impression that what made 3. possible, also added one extra navigation, thus it fixed 1. by navigating once instead of not at all, and created 2. which navigates twice instead of once now.

if you can find me the PR that introduced those changes that might have broken your PR, I will reconsider it

What is bothering me is that I couldn't find the PR which fixed the original issue neither, and we know there must be something.
Is anyone able to identify the change which fixed the old issue? I'd like to investigate if that is related to the new bug. Thanks!
@allroundexperts What would be your opinion about these?
@pecanoro If this remains a regression, I 100% accept it! Just wanted to express why I'm unsure. Thank you!

ahhh, thank you @allroundexperts! I needed some clarification on the status of payments for this one.
I'll let @pecanoro respond to @DanutGavrus' comment before closing this one!

@allroundexperts - i will trust your call on this one. double-checking that no payment should be issued to @DanutGavrus after they outlined these details - https://github.com/Expensify/App/issues/20624#issuecomment-1686573606

Reviewed the details for @allroundexperts. $500 approved for payment via NewDot based on BZ summary.


Replaced for with foreach and replace if ,else if, else  condition with match

In some Places for the loop was used but it can be replaced by the for each.
In One file if else if else was used so I replaced the same with match.

Merged to trunk and 0.10.2.


Winows and MacOs binaries not available via https://firmware.turingpi.com/tpi

Repository readme references prebuilt binaries are available for windows, Linux and macOS. But when navigating to URL: https://firmware.turingpi.com/tpi, per readme only Debian Linux images are available.
From a user experience perspective please either reference https://github.com/turing-machines/tpi/releases (I stumbled across this URL) or upload/link binaries to https://firmware.turingpi.com/tpi.. fyi. fantastic utility, lets make it easy for all tpi2 users to use

thanks for the feedback. Your right, there is still some work to do regarding packaging and making it available via the corresponding channels.
Do you remember where in the documentation there is a mention on the tpi tool available on the firmware.turingpi.com site? then i will update it

soory for the delay, just in the readme https://github.com/turing-machines/tpi/blob/master/README.md

Apologies the is Turning Pi tpi reference https://docs.turingpi.com/docs/tpi-installation

Since Apple hasn't sold any x86-64 hardware in a couple of years, can we get Apple Silicon binary ?

@nhi-vanye to get you going, the easiest way to install it on your mac would be
brew install rustup-init
rustup-init
cargo install --git https://github.com/turing-machines/tpi/

Thanks for that super quick start... took less time than making a cup of tea


Refactored swagger spec from dataset api changes

What
Refactored the swagger spec
How to review
Review the swagger spec
Who can review
anyone

Closing as https://github.com/ONSdigital/dp-import-api/pull/20 supercedes this


Re-enable UI when running on k8s

Remove force-disablement of UI on k8s clusters as work on this will be starting during this Forklift release.

Why not wait until we have a solution in place for the UI authentication? Until then, re-enabling the UI will make the deployment fail.

@fdupont-redhat the deployment of UI actually succeeds , the issue is that UI does not know yet how to handle the absence of oauth2 , we can wait or just have it available for UI devs so they can successfully begin their testing with latest operator.

But they can already enable the UI with feature_ui: true, no?

@fdupont-redhat They can set feature_ui: true and will take precedence but the controlling variable in that conditional is k8s_cluster meaning that only gets processed on OpenShift and not k8s , they will have to force that variable too but will have unintended consequences.


Update appcat postgresql to v0.1.7

Bump version
Update: The new application version is incompatible with the current component therefore a simple bump of version is not enough. The problem of webhook certificates has to be resolved.
Checklist

[x] PR contains a single logical change (to build a better changelog).
[x] Categorize the PR by setting a good title and adding one of the labels:
bug, enhancement, documentation, change, breaking, dependency
as they show up in the changelog.

Idea: As long as we haven't implemented #8 , we should be able to set webhook.enabled=false in the helmValues, that way the latest operator version can be installed, just without webhook support (yet).


Analysis tab: Add "Abstract dyads" option under "Display intervals in matrix as"

Most useful for scales with 4 or fewer step sizes, this feature would get all the step sizes in a scale and display each table entry as a linear combination of the step sizes.

For a scale of the form:
let x = 7/6
let y = 8/7
let z = 9/8
x; y; x; y; z;
stack()

the interval matrix with that setting would look like:
     |  1  |  2  |  3  |  4   |  (5)  |
---------------------------------------
0    |  x  | x+y |2x+y |2x+2y |2x+2y+z|
x    |  y  | x+y |x+2y |x+2y+z|2x+2y+z|
x+y  |  x  | x+y |x+y+z|2x+y+z|2x+2y+z|
2x+y |  y  | y+z |x+y+z|x+2y+z|2x+2y+z|
2x+2y|  z  | x+z |x+y+z|2x+y+z|2x+2y+z|


restamp="false" will fail

If you set restamp="false", the element will be removed and injected from DOM after loading the correct page.
As the default value is false, you can get rid of the attribute, but if you include it, the output doesn't work as expected.

This is the correct behavior of boolean attributes in HTML. For more information see http://stackoverflow.com/a/4139805/2761676


Fix Navbar link

Short description of what this resolves:
The link to the Navbar API Doc was broken
Changes proposed in this pull request:

../../navbar/Navbar -> ../Navbar/ (../../tabbar/Navbar)
this link is on the page twice

Ionic Version: 1.x / 2.x / 3.x

Thanks :)


Mouse scroll events

Sip
On Mar 22, 2015 11:47 AM, "Dwan"<EMAIL_ADDRESS>wrote:

Merged #2 https://github.com/xinix-technology/tshirt-scroll/pull/2.
—
Reply to this email directly or view it on GitHub
https://github.com/xinix-technology/tshirt-scroll/pull/2#event-261431033
.


Max. Station Current should be taken for upper Limit

Hi,
just a small UI improvement suggestion, resulting from upgrade to 32A:
The setting number.wallbox_max_station_current limits the max. available current the station can deliver. In my case this is set to 16A and is a limit given by the power grid.
The two sliders number.wallbox_current_limit and number.wallbox_alb_safe_current should adjust their upper limit according to number. wallbox_max_station_current. Currentl<, these sliders show both always 32A als max. value and allow accidently a setting of  32A, even in wallbox_max_station_current is 16A.
Is just a detail, but during the last few days I changed the value accidently multiple times to 32A and I simply don't know, how this is handled by the wallbox.
Regards,
Waldemar

I think I can only check if we have a 16A OR 32A depending on the license we bought.
I need to check on it.

My problem is little different. I have a 32A license, by my electricity provider only allows 11 kW, so 16A. That's why the box's wallbox_max_station_current is set to 16A and I am not allowed to increase it. My hope was, that the other controls would get their max values based on wallbox_max_station_current.
But if this is not easy to solve, it's no problem. I'll try to build an other UI sufficient for me.

i can make a dirty fix, to add extra slider with max of 16A, but that's only if i don't find a solution for it
but wait, we have here 2 type of value, one is the max station current, so if you set them on 16A, then you don't need to touch them anymore. we have also a max current, where you can set the value of the current power that you want. Or did i mis something?

but wait, we have here 2 type of value, one is the max station current, so if you set them on 16A, then you don't need to touch them anymore.

Yes, that's what I ment. Max station current setting is limiting the power. I even do not present the setting in my UI to prevent an accidential change.

we have also a current_limit, where you can set the value of the current power that you want

Yes, that's one of the parameters I am talking about (the other is wallbox_alb_safe_current).

if your station set max on 16A, and you change your current limit to 17A, it will still go to your max station current of 16A

This might be true, I didn't dare to test it. But my main point was, that the UI does not reflect the correct state. My main point was, to set the max value for this parameter dependent on the setting of max_station_current, so you even can't change the value to 17A.
And - to be precise - it is not just about 16A or 32A: A friend of mine is allowed to charge with 20A max. So he would expect the slides max values be 20A. Just as an example.
But if this is not possible or hard to achieve, I will create my own template number to get the right values presented.
Regards, Waldemar

This might be true, I didn't dare to test it. But my main point was, that the UI does not reflect the correct state. My main point was, to set the max value for this parameter dependent on the setting of max_station_current, so you even can't change the value to 17A.

I haven't tested it yet, but if you want to be sure, you can try to set the max station current to 10 and set the current_limit to 16. And see what the value is. normally it should stay around 10A and never go higher than 16A. if that work then you can be sure that setting on 16A (max station current) and 32A (current_limit) should still stay at 16A

I'll close this ticket, generally, if you set on your station a max of x Amps, your connector should not go over the max amps of the station, even if your connector has higher amps set up
if more discussion need, maybe you can open one on the discussion topic: https://github.com/leeyuentuen/alfen_wallbox/discussions

@mumpf i've reopen a new ticket to solve your initial question,
https://github.com/leeyuentuen/alfen_wallbox/issues/78
there is now a beta version available. It will check if there is a 32A license, if not, it will show only 16A
https://github.com/leeyuentuen/alfen_wallbox/releases/tag/2.7.15


NamesList.txt

https://unicode.org/Public/UCD/latest/ucd/NamesList.txt

	This file is semi-automatically derived from UnicodeData.txt and
	a set of manually created annotations using a script to select
	or suppress information from the data file. The rules used
	for this process are aimed at readability for the human reader,
	at the expense of some details; therefore, this file should not
	be parsed for machine-readable information.


Revert "Don't use timeout package when stopping shim logger"

This reverts commit b26e6abe00d17541647f78c37bbf091cbf797fc3.
Breaks in Kubernetes CI jobs. Both in Kind and in a full cluster as well. Reproduced in https://github.com/kubernetes/kubernetes/pull/91786. Note that 1.3.4 is fine ( https://github.com/kubernetes/kubernetes/pull/91733 )
Context:
(CI issue) https://github.com/kubernetes/kubernetes/issues/91579
(Revert) https://github.com/kubernetes-sigs/kind/pull/1649
Theory:
the timeout was 1 sec before this commit (picked from Default timeout https://github.com/containerd/containerd/blob/master/pkg/timeout/timeout.go#L30) and now it is 12 sec after this commit. This is causing issues. as @liggitt termed it "timing-fragile process interlocks are exciting" :)
Signed-off-by: Davanum Srinivas<EMAIL_ADDRESS>

@dims the commit you're referring to (https://github.com/containerd/containerd/commit/b26e6abe00d17541647f78c37bbf091cbf797fc3) was the 1.3 backport commit  from https://github.com/containerd/containerd/pull/4278. Did you mean to revert https://github.com/containerd/containerd/commit/0dc7c8595627e38ca2b83d17a062b51f384c2025 (from https://github.com/containerd/containerd/pull/4195) ?


🚧 Validates documentation migration

Resolves #58.
Based on https://www.jenkins.io/doc/developer/publishing/documentation guidelines. For the migration to be complete, it requires a README.{md,adoc} file and for the build tool to have the url configured.
Requires to parse build configuration file.

At this moment, only Maven based projects can be handled. Working on Gradle ones

It seems that retrieving the extension configuration from the gradle project is not something that the gradle tooling library is providing (https://discuss.gradle.org/t/how-to-get-the-project-extensions-in-gradle-tooling/6042/4).
The gradle-jpi-plugin is written in Groovy, Java and Kotlin. The extensions is registered here
https://github.com/jenkinsci/gradle-jpi-plugin/blob/c813f75e64a409b65795f35eb79ede6982406e1b/src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiPlugin.groovy#L103
and the JpiExtension would give us the required url (https://github.com/jenkinsci/gradle-jpi-plugin/blob/c813f75e64a409b65795f35eb79ede6982406e1b/src/main/groovy/org/jenkinsci/gradle/plugins/jpi/JpiExtension.groovy#L144-L146).
Based on https://github.com/search?l=Gradle&q=org%3Ajenkinsci+filename%3Abuild.gradle&type=Code, the ecosystem has 83 plugins built with Gradle.

For the record, the current lead to solve the problem with Gradle based plugin is to statically log them in a file on the project and use that file.