WIP: Security Policy

I'd like to get some feedback to lock down the interactions with how the service account defines and enforces policy which results in a security context on the pod.  This contains some types that @pmorie and I came up with that defines a SecurityPolicy object that can be configured with constraints as well as strategies to help enforce and create a security context.
This PR is based on @liggitt 's service account PR, only the last commit contains new code.
@smarterclayton @erictune @pmorie @liggitt - PTAL and see if this is the approach to move forward.  If this is ok then I'll update the design docs with use cases and types.
edit: I've distilled this down to just the design to make it easier.

@bgrant0607 @erictune

reorganized the commits to be in a more sane format for reviewing

@erictune @smarterclayton @pmorie @deads2k @liggitt - I've distilled this down to just the design to gather feedback on the proposal and types.  Please take a look.

@pweil- This needs rebase now that service accounts are in.

Updated

@deads2k @pmorie @liggitt this has been updated to reflect our discussions yesterday.  Please take a look

@liggitt - I think I've addressed your feedback.  PTAL, particularly around admission.  Since we agree that the strategies belong on the SCC I believe that unioning of permissions is not a requirement.  The allocator must find an exact match in order to know what strategies to use (which would in turn look for pre-allocated values).
Real world example is SCC A that allows priv == true and RunAsAny user strat and SCC B that has priv == false and MustRunAsRange user strat.  We need to find exactly which SCC supports the pod.Privileged == true setting in order to validate the existing fields and generate the unset fields so I don't think a union buys us anything.
That said, it will be an administration task to ensure that you cannot have multiple SCCs that support a request.  The allocator would be dumb and just pick the first match.  I believe that this can be avoided by setting the values of pod.container.securitycontext to ensure it runs with the right SCC and is only a problem when it comes to setting some, but not all, of the security context fields and letting the allocator pick how to generate the rest.

@pmorie: de-nitted, ptal

@pweil- Are you going to add treatment for pods that come from a non-apiserver config source (mirror pods) ?

For the record your delousing operation was a success.

@pweil- Are you going to add treatment for pods that come from a non-apiserver config source (mirror pods) ?

That is something I'm still a bit unclear on.  In some of the other conversations, the thought was that if a manifest based pod is created then the creator has administrative privileges and we should trust the security context on the pod definition.  I think the disconnect, for me, is the behavior of the system if the mirror pod subsequently fails admission.  Open to suggestions on this topic.

The kubelet uses an authenticated client to submit mirror pods to the API. As long as that client's credentials identify the kubelet as a user that can submit privileged pods, admission should succeed.
The main thing I've run into that you need to be careful about is modifying the spec of mirror pods in an admission controller. I think that would require the security context of a mirror pod to be fully specified.

If the kubelet is running without an API connection, no mirror pods are created.

@pmorie update based on your most recent feedback.
Added a new commit for types and storage implementation

@liggitt @pweil- we discussed adding a default security context for mirror
pods -- doesn't that allow you to specify?
On Mon, May 18, 2015 at 10:45 PM, Paul Weil<EMAIL_ADDRESS>wrote:

@pmorie https://github.com/pmorie update based on your most recent
feedback.
Added a new commit for types and storage implementation
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/7893#issuecomment-103309834
.

@liggitt @pweil- we discussed adding a default security context for mirror
pods -- doesn't that allow you to specify?

Having an SCC on the kubelet would be fine and allow the kubelet to generate an SC if not specified.  However, that SCC must not use any strategy that tries to look up pre-allocated UIDs, etc since it may not be running with an api connection.  It's basically an admin SCC with RunAsAny.

It's basically an admin SCC with RunAsAny

Which makes sense, given where the static pods came from

Also, to speak with precision, "static pods" are the pods that come from a source manifest file on the kubelet. "mirror pods" are exact replicas of those static pods which the kubelet submits to the API (if the kubelet has an API connection), presumably to help inform the scheduler about things that are consuming resources on the node.

added more implementation details, interfaces for the allocator api, strategies, and updates to the design to support the strategy options.
UC Example 1: the MustRunAs user strategy can support both pre-allocated and static uids by allowing the options to configure an annotation or a static id.  This supports the use case of running all SAs with a unique id which can be pre-allocated and assigned.
UC Example 2: running as an arbitrary id is supported by the RunAsAny strategy which returns whatever is configured in the pod's SC or nil so the image (or default) user will be used.

bump for some eyeballs on the type updates and code (particularly the must run as user strategy that would support pre-allocated UIDS)

What has two eyeballs and a burning need for security context constraints?  THIS GUY

rebased, PTAL @pmorie

I asked a bunch of questions and suggested some alternatives.  The motivation behind my questions was a couple of things:

I want it to be easier for customized kubernetes implementations to plug in their own policy language (e.g. hypothetical AWS IAM extensions for Kubernetes).  this means:

try to keep generic authorization for doing things to opaque objects fairly separate from rules for specific fields in objects (I think having a SecurityContextConstraints object does this)
try to keep service account concept (which various existing auth frameworks might already have) separate from pod specifics (hence my question about why those are entangled

a realization that we will need something quite similar to this for services (#8723) and trying to find avoid a excessive duplication of workflow for services.
ongoing conversations with my company's authz people about lessons learned from our current system, and desire to apply those.  Much more conversation expected along these lines when I review the PR for Policy.

Updated the design based on the feedback that is not still under discussion.

I'd like us to treat the following things in a consistent way:

core authorization: Role, PolicyRule, etc (https://github.com/openshift/origin/blob/master/pkg/authorization/api/v1/types.go)
authorizing creation and use of SecurityContexts (this PR: #7893)
authorizing use of specific fields in Services #8723
authorizing use of hostDir #7925
authorizing use of a secret from a pod #4957
authorizing use of a secret from an ImagePullSecret or ServiceAccount
authorizing use of a TemplateRef in a ReplicationControllerSpec
other things that may come up

I don't think we have that yet, and I don't think we can get there before the pre-v1 code freeze.  So, I think this PR needs to be on hold until after v1.

Ok.  We'll carry this in OpenShift and circle back.

On May 29, 2015, at 2:14 PM, Eric Tune<EMAIL_ADDRESS>wrote:
I'd like us to treat the following things in a consistent way:
core authorization: Role, PolicyRule, etc (https://github.com/openshift/origin/blob/master/pkg/authorization/api/v1/types.go)
authorizing creation and use of SecurityContexts (this PR: #7893)
authorizing use of specific fields in Services #8723
authorizing use of hostDir #7925
authorizing use of a secret from a pod #4957
authorizing use of a secret from an ImagePullSecret or ServiceAccount
authorizing use of a TemplateRef in a ReplicationControllerSpec
other things that may come up
I don't think we have that yet, and I don't think we can get there before the pre-v1 code freeze. So, I think this PR needs to be on hold until after v1.
—
Reply to this email directly or view it on GitHub.

https://github.com/openshift/origin/pull/2856

ok to test

@pweil- please tag me when this rebased.

@deads2k - rebased to 96828f203c8d960bb7a5ad649d1f38f77ae8910f as requested

@deads2k - rebased to 96828f2 as requested

Got it.  Thanks.

ok to test
On Wed, Jul 1, 2015 at 10:35 PM, Kubernetes Bot<EMAIL_ADDRESS>wrote:

Can one of the admins verify that this patch is reasonable to test? (reply
"ok to test", or if you trust the user, reply "add to whitelist")
If this message is too spammy, please complain to ixdy.
—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/7893#issuecomment-117879175
.

@pweil- The useHostDir flag needs to be renamed to useHostPath...

needs rebase.

rebased.

I'd like us to treat the following things in a consistent way:
core authorization: Role, PolicyRule, etc (https://github.com/openshift/origin/blob/master/pkg/authorization/api/v1/types.go)
authorizing creation and use of SecurityContexts (this PR: #7893)
authorizing use of specific fields in Services #8723
authorizing use of hostDir #7925
authorizing use of a secret from a pod #4957
authorizing use of a secret from an ImagePullSecret or ServiceAccount
authorizing use of a TemplateRef in a ReplicationControllerSpec
other things that may come up

@pmorie I'd like to come to an agreement on the base structures of the objects that constrain policy.  I think the key missing item is how it would integrate with the Role/Policy Rule/etc objects.  The authorization components are pretty simple after that (as an admission rule it is easy to allow/disallow things based on the settings) and the object can be made to have different sections about authorized N types of api objects, not just pods/containers.

@pmorie give some love to @pweil- here? Where do we stand?

I'm on vacation this week (:house: :mouse:) and will take a look next week.

Somewhere in this or a related thread, I said we should not merge this issue until we figured out a grand unified theory of policy that would encompass both the SecurityContextConstraint feature and the Policy/RoleBinding features into one object.  I haven't come up with a way to do that.  So, I think it is time to unblock this PR.
Also, @mikedanese convinced me that the two-phase approach is easier for users to reason about and has a good track record at our company.
By two-phased approach, I mean:

authorizer module checks if user is authorized to create/update objects of type X in this namespace at all.
an admission controller checks the namespace to find the exactly one SecurityContextConstraint for that namespace, or the default one for the cluster.  It rejects changes to objects that have specific disallowed fields.

I realize this was the original proposal and that I previously pooh-pooh-ed it.  I am un-pooh-pooh-ing it. :hankey:

One thing @mikedanese and I talked about is whether the SecurityContextController is only meant to control allowed values in the pod.spec.securityContext.  If so, then what will the objects be called that control what values are allowed for pod.spec.volumes[].hostDir, or pod.spec.ports[].hostPort.
Is multiple objects better or a single object. e.g.
   scc SecurityContextConstraints
   allowHostPort bool
   allowHostDir bool
   ...

@mikedanese you said you had time to review this.  Assigning to you.

I would really prefer if this was PodSecurityPolicy and was comprehensive
(as much as possible).
On Sep 10, 2015, at 7:07 PM, Eric Tune<EMAIL_ADDRESS>wrote:
One thing @mikedanese https://github.com/mikedanese and I talked about is
whether the SecurityContextController is only meant to control allowed
values in the pod.spec.securityContext. If so, then what will the objects
be called that control what values are allowed for
pod.spec.volumes[].hostDir, or pod.spec.ports[].hostPort.
Is multiple objects better or a single object. e.g.
scc SecurityContextConstraints
allowHostPort bool
allowHostDir bool
...

 —
Reply to this email directly or view it on GitHub
<https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-139407295>.

I would really prefer if this was PodSecurityPolicy and was comprehensive

Yes, I'd like to get this renamed in this PR.  I think it more accurately reflects what we're trying to achieve and extends well to other objects that we want to control in the same pattern that don't have a SecurityContext (like services).  I have completed the work in downstream but need to roll it up here.  I'll include the changes for the other comments as well.

We don't want to delete non-conforming objects after an SCC is changed.  And we need to be able to change them, particularly to allow adding SCCs to pre-existing clusters.  So, we should tell users whether  objects created prior to creation/modification of the SCC conform to it.
Therefore, suggest having both SecurityContextConstraints.Spec and a SecurityContextConstraints.Status.
The Spec would have all the things that are in the object now.
The Status would have at least a Condition.  The Condition would be Status True when all the objects that the SCC managed would pass admission control under its current Spec, otherwise False or Unknown.

Re-evaluating compliance requires having all the inputs. If the context user/groups is one of the inputs, that would need to be recorded for later re-evaluation

@mikedanese - rebased, renamed to PodSecurityPolicy, removed a lot of the extra naming on the api objects.

Couple comments on the API changes:

Please break the top level object into a Spec and Status per api convetions. It's okay if an object only has a spec (see limit range)
we should put this in the experimental API to soak

@mikedanese @smarterclayton @liggitt  - thanks for all the feedback so far.  Here's a summary based on our convo up to this point for things that are addressed in the round1 feedback commit or addressed in comment history:

host path specific validation - punting on this until later iterations https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39240825
break up into spec and status - https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-140167181
break up and reorg constant blocks - https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39451945
rename strategy types - https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39452008
update godoc - https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39539575
move to experimental api - https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-140167181
support port ranges - https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39418844

Incomplete

validation for host port ranges in the provider - pending approval of new api objects

if it would help this along I can remove the provider and strategy implementations so this is just api and submit the provider logic separately.

if it would help this along I can remove the provider and strategy implementations so this is just api and submit the provider logic separately.

+1 that makes sense to me. Can we put this in the experimental API? I'm going to continue reviewing this after v1.1 is cut (monday). I'd really like to get this in as experimental for v1.2.

+1 that makes sense to me. Can we put this in the experimental API? I'm going to continue reviewing this after v1.1 is cut (monday). I'd really like to get this in as experimental for v1.2.

The current iteration has this in experimental.  +1 to targeting v1.2.  Thanks for the update

@mikedanese - updated to be api only

What is the plan for restricting use of volume plugins?  Does that belong in PodSecurityPolicy?
Volumes are in Pods.
For example, if you ask a kubelet to mount a fiber channel volume, it'll do that, as root.  Not necessarily something you want all users to be able to do, but maybe certain users and/or namespaces can access certain volumes.
Fine if the answer is "probably belongs in PodSecurityPolicy but not in this PR".
@pmorie @rootfs

For object level policy in Openshift, the a set of allowed actions -- a Role -- is a separate type from the list of people who can act in that role -- a RoleBinding.  This allows reuse of Roles, I gather.
In PodSecurityPolicy, the two parts are in one object.  Everyone okay with that discrepancy?

That was the desire.
On Sep 30, 2015, at 10:24 AM, Eric Tune<EMAIL_ADDRESS>wrote:
What is the plan for restricting use of volume plugins? Does that belong in
PodSecurityPolicy?
Volumes are in Pods.
For example, if you ask a kubelet to mount a fiber channel volume, it'll do
that, as root. Not necessarily something you want all users to be able to
do, but maybe certain users and/or namespaces can access certain volumes.
Fine if the answer is "probably belongs in PodSecurityPolicy but not in
this PR".
@pmorie https://github.com/pmorie @rootfs https://github.com/rootfs
—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-144325244.

We went back and forth on this - part of the role binding argument was that
end users (project admins) would be the ones who can bind, but cluster
admins are the ones who define roles.  Down the road this needs to be more
of a capability model - I have the capability to run a pod with privileged
and the capability to give rights to others, therefore I can add another
user to the PSP.  It also matches some of the discussions we had a long
time ago about pod templates (I own the template, I identify who can edit
it).
So in the short term it seemed parsimonious to model this as a unified item
because it is very much a cluster admin role and it is the most succinct
way to encapsulate the problem today.
On Sep 30, 2015, at 10:33 AM, Eric Tune<EMAIL_ADDRESS>wrote:
For object level policy in Openshift, the a set of allowed actions -- a Role
https://github.com/openshift/origin/blob/70015e4a6f9b821ccd5a01134054286e3813b4eb/pkg/authorization/api/v1/types.go#L39
-- is a separate type from the list of people who can act in that role -- a
RoleBinding
https://github.com/openshift/origin/blob/70015e4a6f9b821ccd5a01134054286e3813b4eb/pkg/authorization/api/v1/types.go#L50.
This allows reuse of Roles, I gather.
In PodSecurityPolicy, the two parts are in one object. Everyone okay with
that discrepancy?
—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-144327384.

What is the plan for restricting use of volume plugins? Does that belong in PodSecurityPolicy?
Volumes are in Pods.
For example, if you ask a kubelet to mount a fiber channel volume, it'll do that, as root. Not necessarily something you want all users to be able to do, but maybe certain users and/or namespaces can access certain volumes.
Fine if the answer is "probably belongs in PodSecurityPolicy but not in this PR".

Yes, it belongs here.  I think having a slice of allowed types could probably be sufficient for this.  I'll add it in.

PR is updated.  Rather than a slice I went with a struct.  I didn't see the benefit of a slice since volume sources are not typed with a string.  This also gives room to expand volume security policies if we need it without adding an additional object.  PTAL

@erictune

For example, if you ask a kubelet to mount a fiber channel volume, it'll do that, as root. Not necessarily something you want all users to be able to do, but maybe certain users and/or namespaces can access certain volumes.

Fine if the answer is "probably belongs in PodSecurityPolicy but not in this PR".

Bingo.

Bingo

https://github.com/pweil-/kubernetes/blob/security-policy/pkg/apis/experimental/types.go#L561

I missed that that was added.  My bad!

reminder: host volume plugin locking down based on directories
https://github.com/kubernetes/kubernetes/pull/13524#issuecomment-153414962

We discussed that in https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39240825 and https://github.com/kubernetes/kubernetes/pull/7893#discussion_r39418754

reminder: host volume plugin locking down based on directories

We should address this before PodSecurityPolicy leaves alpha

We should address this before PodSecurityPolicy leaves alpha

Agree.  I should have time to make it back to this PR when our current release is wrapped up.

/cc @sttts

@pweil- what's the timing on this?

I mostly like this PR, but I would like it if PodSecurityPolicy (PSP) did not contain the list of users and groups that can use the PSP.  I would rather if the Authorization Policy (openshift or otherwise) made the decision of which users/groups can use which PSPs.  I wrote up how I think that would work in a new issue: #17637.

@pweil- what's the timing on this?

Back from PTO today.  This is tops on my list after some administrative items.  Will review #17637 and rebase.

Is there some way we can combine this with #18262 ?

Is there some way we can combine this with #18262 ?

I started reading through your PR yesterday.  I think there are enough similarities to at least identify where things may overlap and consider combining if possible.  I think the main difference that I have seen so far is in the way the policy is determined to be applicable to the submitted resource.
In this proposal you are granted access to specific security policies and may end up using 0, 1, or none of them.  This may still be applicable using selectors as #18262 proposes assuming the selector takes user information into account but I need to think about it a bit more.

@mikedanese - please take a look at the api commit again.  I've updated to use a slice in the volume section and added a binding api object to decouple the users and groups from the policy object.  Let me know what you think.  If that looks good I'll round out the storage for bindings so this can get moving.

@davidopp @pweil-
PodSecurityPolicy is about saying no to pods that set fields to values that are not allowed.  I couldn't tell from the PodPolicy proposal if or when a user would be prohibited by PodPolicy from using a particular label or QoS.   So, they seem different.

Although we have realized and implemented that PSP should also be
capable of defaulting prior to rejection (specifically around
runAsUser which can be delegated to docker but which is considered
unsafe).

Although we have realized and implemented that PSP should also be
capable of defaulting prior to rejection (specifically around
runAsUser which can be delegated to docker but which is considered
unsafe).

Yes, I think the best bet here is to get the api types in and we can decide on the admission/strategy implementation how we want defaulting to work.  In our OpenShift implementation the strategies provide defaulting values, we may decide to use that or go with PodPolicy to provide it and leave PSP as a validation mechanism only.

@mikedanese - please take a look at the api commit again. I've updated to use a slice in the volume section and added a binding api object to decouple the users and groups from the policy object. Let me know what you think. If that looks good I'll round out the storage for bindings so this can get moving.

@mikedanese checking in on this to see if you've had a chance to glance at the binding object

@pweil-, I spent some time discussing how these could be bound to users/groups with @erictune, @lavalamp, and @deads2k today. We may have a way to avoid a second binding object, and instead make use of the subjectaccessreview/resourceaccessreview mechanism @deads2k is working on upstreaming. Will touch base tomorrow.

Agree with what Jordan just said.
I have an idea on how we can break this into smaller steps.
Step 1a: (pweil-) commit PSP type definition, as a non-namespaced object
(like ClusterPolicy)
Step 1b: (pweil-) commit an admission controller that evaluates all Pods
against the cluster-level PSPs. (No differences across namespaces)
Step 1c: (erictune) advise users about the need to create one or more PSPs
before enabling this new admission controller.
At this point, Kubernetes users now have the ability to set allowed PSPs on
a cluster basis.  This is an improvement over before.
Step 2a: (Eric, David, Jordan, pweil-) nail down the mechanism that we
talked to today, and which Jordan just alluded to.  This means adding
can-use to all supported Authorizer modules.
Step 2b: (tbd) add a second PSP admission controller to check whether a
namespace, or users/groups in that namespace can use a PSP, using 2a.
Step 2c: (tbd) advise users about need to add appropriate can-use policies
to all roles/namespaces before upgrading from old Admission controller (1b)
to this new admission controller (2b).
At this point, Kubernetes users can now control PSP usage on a per user,
per group, per role and/or per namespace basis.
On Thu, Jan 7, 2016 at 6:34 PM, Jordan Liggitt<EMAIL_ADDRESS>wrote:

@pweil- https://github.com/pweil-, I spent some time discussing how
these could be bound to users/groups with @erictune
https://github.com/erictune, @lavalamp https://github.com/lavalamp,
and @deads2k https://github.com/deads2k today. We may have a way to
avoid a second binding object, and instead make use of the
subjectaccessreview/resourceaccessreview mechanism @deads2k
https://github.com/deads2k is working on upstreaming. Will touch base
tomorrow.
—
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/7893#issuecomment-169872400
.

A couple of easy-to-address comments, then this looks good to go in.

Cleaned up validation and type defintions
Reduced UID strategy options based on our experiences with FSGroup and SupplementalGroups

Basically as long as you can define n ranges there is no need for a specific "must run as X id" - just make a range that starts and ends with the same value

Of note, in recent discussions we have found we needed more than what the allowed capabilities slice will provide.  There seems to be 3 items than need covered

caps you're allowed to add but not defaulted
caps that you get by default but can drop explicitly
caps you must drop

I can either update this to include those fields (and others we have added since this proposal started) or remove caps from this proposal and do it as a follow up so we can get the mechanics in with less complexity.  Thoughts?

@pmorie - any opinion on the comments above?  I'm going to begin work on the admission controller piece

@erictune ready for your eyes again

rebased.  @erictune let me know if anything is holding this up for the api type going in.  Thanks!

lgtm.
feel free to reapply or have someone reapply lgtm after you rebase.

@bgrant0607 just said this has to go into extensions/v1beta1 for at least a release before it can go into legacy api group and v1.

Oh, duh, it is in extensions/v1beta1. Please ignore last comment.

:-)  whew, thought there was some new api piece I missed.  Will rebase tonight.  tyvm!

IIUC, when this PR goes in, we have the type and endpoint, but no actual enforcement.
Is that enforcement going to come in soon, so it gets into 1.2?  If not, thats understandable, but we should disable this type by default if it does not do anything.

not likely by tomorrow.  I will disable it for now and add enabling it by default in the next PR with the admission controller

SGTM

Special award for longest-lived PR that eventually merged!
Almost 9 months old!  This PR can crawl and eat solid food! :baby: :baby_symbol:

:smiley: rofl

How much work is it to write an admission controller that looks at PSPs but treats all users and groups the same?

not a ton.  Can probably be done by the end of next week based on my other commitments.  It would include:

admission controller
strategy implementations
provider implementation
resource enablement

Most of this is just a straight migration from what is already in OpenShift and whittling it down to be less specific.

Aaand it broke the build:
07:24:01 /workspace/kubernetes/docs/api-reference is out of date. Please run hack/update-api-reference-docs.sh
07:24:01 FAILED

I'll send a fix in a second.

thanks @gmarek - hrm I was running update-all/verify-all after every rebase I'm surprised it broke.  Apologies

Np:) It was tempting to revert this PR, just to make it hang there a little longer ;)

It was tempting to revert this PR, just to make it hang there a little longer ;)

you have no idea how glad I am that you didn't.  :beers:

@gmarek - crap, this does need reverted.  It does not have my changes for disabling the resource by default and the comment updates that were requested.  New PR or revert and fix this one?
https://github.com/kubernetes/kubernetes/commit/3cfb090939c06228feab5ed07e2d4e49b0489065#diff-c47934bf31679532191ed2b519d74399R591

https://github.com/kubernetes/kubernetes/pull/20721


[dependencies] Upgrade to webpack 5

Dependabot could not automatically merge webpack but there is a new major version and eventually, when its more mature, we should evolve.  Until then,  this is an issue to reference as we close any lingering dependabot merges related to webpack utilities.

Looks like storybook doesn’t yet officially support webpack 5
https://github.com/storybookjs/storybook/issues/9216

This is happening as part of https://github.com/trussworks/react-uswds/pull/1544


How to override the css styles

I have used on the Cascade Style Sheet and try to override the style.
but cannot override to write the styles in html page
How to override to write any sample code give.

Seems like a duplicate of #54.


ArgumentError: unexpected value at params[:server_side_encrytion]

I recently upgraded to 1.3.0 (with carrier wave 1.2.2 and aws-sdk-s3 1.8.0)
Our code that used to work on version 0.4.1 (with carrier wave 1.2.1 and aws-sdk-v1 1.57.0) we're seeing this error
ArgumentError: unexpected value at params[:server_side_encrytion].
It seems pretty deep in the stack...
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/param_validator.rb:13 in validate!
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/plugins/param_validator.rb:23 in call
/gems/aws-sdk-core-3.15.0/lib/seahorse/client/plugins/raise_response_errors.rb:14 in call
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/plugins/sse_cpk.rb:22 in call
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/plugins/dualstack.rb:26 in call
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/plugins/accelerate.rb:35 in call
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/plugins/jsonvalue_converter.rb:20 in call
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/plugins/idempotency_token.rb:17 in call
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/plugins/param_converter.rb:24 in call
/gems/aws-sdk-core-3.15.0/lib/aws-sdk-core/plugins/response_paging.rb:10 in call
/gems/aws-sdk-core-3.15.0/lib/seahorse/client/plugins/response_target.rb:23 in call
/gems/aws-sdk-core-3.15.0/lib/seahorse/client/request.rb:70 in send_request
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/client.rb:5462 in put_object
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/file_uploader.rb:42 in block in put_object
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/file_uploader.rb:49 in open_file
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/file_uploader.rb:41 in put_object
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/file_uploader.rb:34 in upload
/gems/aws-sdk-s3-1.8.0/lib/aws-sdk-s3/customizations/object.rb:252 in upload_file
/gems/carrierwave-aws-1.3.0/lib/carrierwave/storage/aws_file.rb:59 in store
...
up to our code...

I found this related post https://itsecureadmin.com/2017/10/ruby-aws-sdk-strikes-again/.
I'm going to try to pass the param in with "AES256", but it would be nice if it just didn't do server side encryption when i send in nothing.
Is this something you have seen?  And is there any chance of a fix?
Thanks, in advance

I may get to it but don't hold your breathe.  It was easier to backdate the gems a bit and get stuff working.  If i tackle it, i'll definitely submit a PR.


💡 Support users who like GUIs

Summary
My friend is giving Manjaro Sway another go, but he is inexperienced when it comes to configs, and I have to concur that reading up on them all the time before writing them is no faster than reaching for the mouse. So we should either preinstall some GUIs (they don't harm people who don't use them) or create really intuitive TUIs (which for some reason are rare, I dunno if everybody just writes his private helpers or if most people really type out those command sequences like lsblk + mount or swaymsg -t get_inputs + swaymsg input ... by hand...)
Drawbacks
Potential confusion through conflicting configs.
Implementation idea
So I would like to propose preinstalling these packages and adding .desktop-files with appropriate aliases so users find them:

sway-input-config aka Keyboard Layouts aka Mouse and Trackpad
swaysettings-git (would need config adjustments on includes: https://github.com/ErikReider/SwaySettings#install) aka Sway System Settings aka Bluetooth Manager aka User Management aka Default Applications aka Wallpaper Configurion (basically linking to every unique module not available otherwise, like on other desktop environments)
nwg-displays --outputs_path $XDG_CONFIG_HOME/sway/config.d/outputs.nwg-displays.conf (using that filename so it takes precedence over a potential outputs.conf) aka Display Management aka Monitor Screens

Though we have to make sure to avoid confusion here - sway-input-config has more detailed keyboard settings, but swaysettings-git has a module for basic keyboard layout management as well.
I also just found https://github.com/nwg-piotr/nwg-shell-config which we should definitely check out.
Will you be able to contribute a PR for this?

[ ] Yes I will be able to get that feature a lot quicker.

I like the idea. ngw-shell-config looks pretty nice - perhaps we could make a more detailed plan once you tried it out?

good point, we will try when there is room again

Maybe it is time for us to embrace nwg-shell completely and maybe even consider hyprland?
I for one are not married to sway ^^ https://nwg-piotr.github.io/nwg-shell/

If I wanted a full-fledged gnome-like session, why shouldn't I use Gnome? Hyperland looks interesting tho, but well, that probably is another distro.

well, what I really like is the keyboard-first and tiling-first approach, which I do not think GNOME fulfills

We could explore installing hyprland in parallel. Has anyone got a basic config that we could use as a starting point? I could imagine we could try to rebuild the whole look&feel of manjaro sway relatively easily on hyprland, at least to doesn't look to differently and ppl seem to use mainly the same tools around it.

Exploring hyprland sounds even more sensible now, I am not married to backwards-compatibility as apparently sway wants to be: https://github.com/swaywm/sway/issues/8241

fair point - I also heard reports of people switching from hyprland to sway because of stability: https://www.reddit.com/r/swaywm/comments/1gt79nd/sway_more_stable_than_hyprland/
so maybe sway is a good choice for somebody who does not want updates to force him to tinker around and fix configs ;)


Add a warning about increased memory consumption for audit logging

Follow-up of https://github.com/kubernetes/kubernetes/issues/60500
/cc @ericchiang @tallclair @sttts @shyamjvs

@crassirostris is this for 1.10? Looks as though the related issues in k8s/k8s are ... if so, can you please redirect to the release-1.10 branch? Thanks!
/assign

@Bradamant3 Done, thanks!

/lgtm
/approve
n.b. Related docs PRs have been tech reviewed, so skipping separate lgtm bc release time.


Scan. a Physical Receipt

Reason
As a user I need to scan physical receipts so that I can send this data to TIKI
Description
In the retailer screen, the scan button should call the Scan method in Capture Receipt (through Capture Service) and return the scanned receipt.
Success Criteria

[ ] a scanned receipt is returned from the method called in scan button

Additional Information
No response

done in #59


Limit number of e-cash notes per denomination

#1140 introduces max_target_denomination_sets as the maximum number of e-cash notes issuable per denomination in one output. We need to figure out what this number should be in relation to the target number of notes per denominations on the client side and if there should be exceptions for the highest used denomination (to allow adaptive coin selection for increased privacy).

I set it to default at 3 based on the amount of additional signing time it added https://github.com/fedimint/fedimint/pull/1072
But it obviously varies with the amount sizes too.

I think this can be closed after the work @jkitman has been doing. There is still #116 which is hinted at here, but that's a separate issue (although it will interact with this one).


improvements from internal review

Docs Build status updates of commit bf426fe:
:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

docs/develop/auth-external-add-ins.md
:white_check_mark:Succeeded

docs/develop/overview-authn-authz.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

Docs Build status updates of commit 8eafeff:
:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

docs/develop/auth-external-add-ins.md
:white_check_mark:Succeeded

docs/develop/overview-authn-authz.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.

Docs Build status updates of commit 50c0d92:
:white_check_mark: Validation status: passed

File
Status
Preview URL
Details

docs/develop/auth-external-add-ins.md
:white_check_mark:Succeeded

docs/develop/overview-authn-authz.md
:white_check_mark:Succeeded

For more details, please refer to the build report.
Note: If you changed an existing file name or deleted a file, broken links in other files to the deleted or renamed file are listed only in the full build report.


fix: Place cursor at the end of the replaced selection

Details
In the room name input, place the cursor at the end of the replaced selection when it contains spaces.
Fixed Issues
$ https://github.com/Expensify/App/issues/33652
PROPOSAL: https://github.com/Expensify/App/issues/33652#issuecomment-1870537268
Tests
Same as QA
Offline tests
Same as QA
QA Steps

Create a room named e.g. "room"
Select the name
Replace the selected name with a value that contains spaces (e.g. "room name")

[ ] Verify the cursor is placed at the end of the pasted text
[ ] Verify that the spaces in the pasted name were turned into dashes "-"

Select any symbol(s) in the name and press space

[ ] Verify the cursor is placed at the end of the replaces selection
[ ] Verify the selected value turned into a dash "-"

PR Author Checklist

[x] I linked the correct issue in the ### Fixed Issues section above
[x] I wrote clear testing steps that cover the changes made in this PR

[x] I added steps for local testing in the Tests section
[x] I added steps for the expected offline behavior in the Offline steps section
[x] I added steps for Staging and/or Production testing in the QA steps section
[x] I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[x] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
[x] I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).

[x] I included screenshots or videos for tests on all platforms
[x] I ran the tests on all platforms & verified they passed on:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

[x] I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
[x] I followed proper code patterns (see Reviewing the code)

[x] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
[x] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[x] I verified that comments were added to code that is not self explanatory
[x] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[x] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method

[x] If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:

[x] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[x] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[x] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[x] I verified the JSDocs style guidelines (in STYLE.md) were followed

[x] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[x] I followed the guidelines as stated in the Review Guidelines
[x] I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
[x] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[x] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[x] I verified that if a function's arguments changed that all usages have also been updated correctly
[x] If any new file was added I verified that:

[x] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[x] If a new CSS style is added I verified that:

[x] A similar style doesn't already exist
[x] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))

[x] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
[x] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[x] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[x] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
[x] If the PR modifies the form input styles:

[x] I verified that all the inputs inside a form are aligned with each other.
[x] I added Design label so the design team can review the changes.

[x] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[x] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native
https://github.com/Expensify/App/assets/12595293/afe1de66-5ea4-43a4-8495-975d851d81b6

Android: mWeb Chrome
https://github.com/Expensify/App/assets/12595293/9699cfd1-32d1-4517-9794-dbec1b33a1e3

iOS: Native
https://github.com/Expensify/App/assets/12595293/55bfbf37-cc21-482c-97e5-a43a9d80d6b3

iOS: mWeb Safari
https://github.com/Expensify/App/assets/12595293/311fed27-f029-43e9-9afa-69c53b90fb35

MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/12595293/95f41d41-302c-4d48-94a5-3b590396e6dd

MacOS: Desktop
https://github.com/Expensify/App/assets/12595293/34651553-255f-4fbc-b321-2ab52e8d0afd

Reviewer Checklist

[ ] I have verified the author checklist is complete (all boxes are checked off).
[ ] I verified the correct issue is linked in the ### Fixed Issues section above
[ ] I verified testing steps are clear and they cover the changes made in this PR

[ ] I verified the steps for local testing are in the Tests section
[ ] I verified the steps for Staging and/or Production testing are in the QA steps section
[ ] I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
[ ] I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)

[ ] I checked that screenshots or videos are included for tests on all platforms
[ ] I included screenshots or videos for tests on all platforms
[ ] I verified tests pass on all platforms & I tested again on:

[ ] Android: Native
[ ] Android: mWeb Chrome
[ ] iOS: Native
[ ] iOS: mWeb Safari
[ ] MacOS: Chrome / Safari
[ ] MacOS: Desktop

[ ] If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
[ ] I verified proper code patterns were followed (see Reviewing the code)

[ ] I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
[ ] I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
[ ] I verified that comments were added to code that is not self explanatory
[ ] I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
[ ] I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
[ ] I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
[ ] I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
[ ] I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
[ ] I verified the JSDocs style guidelines (in STYLE.md) were followed

[ ] If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
[ ] I verified that this PR follows the guidelines as stated in the Review Guidelines
[ ] I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
[ ] I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
[ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
[ ] If a new component is created I verified that:

[ ] A similar component doesn't exist in the codebase
[ ] All props are defined accurately and each prop has a /** comment above it */
[ ] The file is named correctly
[ ] The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
[ ] The only data being stored in the state is data necessary for rendering and nothing else
[ ] For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
[ ] Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
[ ] All JSX used for rendering exists in the render method
[ ] The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions

[ ] If any new file was added I verified that:

[ ] The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory

[ ] If a new CSS style is added I verified that:

[ ] A similar style doesn't already exist
[ ] The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)

[ ] If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
[ ] If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
[ ] If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
[ ] If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
[ ] If the PR modifies the form input styles:

[ ] I verified that all the inputs inside a form are aligned with each other.
[ ] I added Design label so the design team can review the changes.

[ ] If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
[ ] If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.
[ ] I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

Hey @getusha, I see you've completed your checklist, but did not approve the PR yet – is there anything missing?

:hand: This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

🚀 Deployed to staging by https://github.com/marcaaron in version: 1.4.25-0 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
failure ❌

🕸 web 🕸
success ✅

🚀 Deployed to production by https://github.com/mountiny in version: 1.4.25-10 🚀

platform
result

🤖 android 🤖
success ✅

🖥 desktop 🖥
success ✅

🍎 iOS 🍎
failure ❌

🕸 web 🕸
success ✅


SAR Watcher

Hello Dr. Liu,
I just wanted to ask if the SAR tool is still in development, as I am having trouble getting it to show anything.
Many Thanks!
-Katya

The SAR tool should be up and functional in this version. What is your setup for showing SAR map?


[FE] GW-03: Create base layout components

Component for creation:

header
login
forgot password
reset password
reclam banner place
catalog place
blog news place


including another file

Describe what you want to achieve.

I would like to include another json file within my json. Is there a way of doing it?

Describe what you tried.

the schema syntax would be something like
$ref:"/path/to/my/file.json"

Describe which system (OS, compiler) you are using.
any

Describe which version of the library you are using (release version, develop branch).
latest

This library didn't support Json Reference now, so I'm afraid it can't do that.

ok, provided that we implement it within our wrapper around nlohmann (so that is it not an implementation problem) could u hint about what would be the "jsonic" way of achieving what we seek?
I mean, implementing our own special handling of "$ref" would be a good choice or should we go for a different approach to be able to include one json inside the other

@dota17 is right, the library does not support this.

Dear @nlohmann @dota17 i appreciate that (i also read it in your docs).
We have a wrapper on the top of your lib which allows us to implement extra stuff. I was asking for a hint of what you think should be the best strategy to achieve it, not from a programming point of view but rather from a "json-organizational" point of view.

You could do it either inside the parser writing your own SAX interface or you do it as post processing step after parsing. For the former, you may want to have a look at the json_sax_dom_parser (https://github.com/nlohmann/json/blob/develop/include/nlohmann/detail/input/json_sax.hpp#L145) parser. With some work, you should be able to add the required logic to replace $ref values.


Fix typo IsPullReuqestBroken -> IsPullRequestBroken

Everything is in the title. Probably not the most useful PR, but quality matters right? :-)

Codecov Report

Merging #4578 into master will not change coverage.
The diff coverage is 0%.

@@          Coverage Diff           @@
##           master   #4578   +/-   ##
======================================
  Coverage    20.2%   20.2%
======================================
  Files         156     156
  Lines       31140   31140
======================================
  Hits         6292    6292
  Misses      23890   23890
  Partials      958     958

Impacted Files
Coverage Δ

routers/repo/pull.go
0% <0%> (ø)
:arrow_up:

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 d572336...2e6c1d8. Read the comment docs.

LGTM


⚠ ClamAV has degraded performance

In fcc70e9, ClamAV (https://spamassassin.apache.org/) experienced degraded performance:

HTTP code: 200
Response time: 70 ms

Resolved: ClamAV performance has improved in 2f9a1a0.


Task list component (x-gov collaboration)

This component is being developed collaboratively by the Task list group.
The aim is to enable the existing Task list pattern, which is widely in use by services, to be built using govuk-frontend components. (At the moment, this requires custom code, such as is included in the GOV.UK Prototyping Kit).
Our approach is to define a component as being just the list of tasks and their statuses, rather than encompassing the whole pattern, which may include many of these lists along with section headings and additional content.
This means that the numbered headings and indented list is out-of-scope.  This could be implemented at a future date through additional modifiers, override classes, or a separate "numbered sections" component (which might have additional uses outside of the Task list pattern).
There are multiple branches to this work, which are being considered by separate groups:

the overall component, including the macro and the generated HTML
the 'status' label given to tasks
guidance on how to use both the component and the overall pattern

We will also try and collate existing user research to feed into each of these.
Some of the outstanding questions are:
Component

[x] What should the component be called?
Sticking with "task list" as the name is well understood.
[x] How should the Nunjucks macro work?
[x] Should the HTML be a
 list, a
 list (like Summary list) or a
?
Think we have established that a
 is best.
[ ] Are the aria-describedby relationships useful?
Needs to be tested by an accessibility expert, but we’ve keep this in for now on the assumption that it is helpful.
[x] Should we also support an additional line of description/hint text for each task?
Think we've decided that there are use-cases for this.

Status tags

[x] How should the status labels appear visually?
[x] Should it use the Tag component, or define its own output?
[x] Should the status labels be UPPERCASE or not?
[ ] What are the recommended words to use for the status tags?
[ ] What are the recommended colours to use for status tags?

Guidance

[x] What should the overall pattern be called?
[ ] If a task cannot be started yet, should there still be a link or not?
[ ] How should a service be broken up into tasks?
[ ] How should tasks be grouped into sections?
[ ] Should tasks be explicitly marked as 'completed' by a user, or should they get completed some other way?
[ ] What should services do if some tasks depend on other tasks?
[ ] What should happen if a user clicks on a task they previously completed?

Thanks to
@ImranH-GDS @owenatgov @MK-NG @steve-oconnor @Dawdre @lfdebrux @frankieroberto

One of the questions for this component is what do we call the param that holds the array of things with statuses?
Our initial instinct was to use rows to match Summary List and because they look like rows (horizontally divided).
However I've done a quick check of what existing components use:
Accordion: items
Breadcrumbs: items
Checkboxes: items
Error summary: errorList
Footer: items, navigation
Header: navigation
Radios: items
Summary List: rows, actions
Table: rows, head
Tabs: items
Based on this, I wonder whether it would be more consistent to use items instead? That would also not tie us to a particular visual style, should the component evolve in future to render differently.

GDS wrote a summary back in 2017 of some of the pros and cons of using Tables, Definition lists and Lists.

We might want to add role=list to the
, see https://github.com/alphagov/govuk-prototype-kit/issues/1009 cc: @edwardhorsford

Here’s the original PR which added the task list to the Prototyping Kit.

One of the questions for this component is what do we call the param that holds the array of things with statuses?
[...]
Based on this, I wonder whether it would be more consistent to use items instead? That would also not tie us to a particular visual style, should the component evolve in future to render differently.

I've switched to items as the param name in 181273d78778ca14a7e5b923607dd483f101c426, based on the following rationale:

6 components use items vs 2 that use rows
Consistency with the HTML tag name (
 - "list item") and CSSS class name (govuk-task-list__item)
More generic, not tied to visual appearance

@owenatgov many thanks for reviewing this! It is still work-in-progress, but now is definitely a good time for us to start to get detailed reviews and to refine the code.

@owenatgov thanks for the follow-up review! (Sorry it's take a while to respond, I was on holiday).
And no worries about coming across snipey, these comments are great and I really value them! Let me know if you spot anything else!

On the task list pattern page the task list summary (ie 'Application incomplete. You've completed x of x tasks.') is just an image. It would be useful if this was a code sample at least.

On the task list pattern page these three links are all broken:

Hi @frankieroberto would you mind rebasing this PR?
It's missing some recent commits organising our dependencies from devDependencies for Heroku review apps. Don't think the PR has “Allow edits and access to secrets by maintainers” ticked so can't do it for you (sorry)
For now though I've pushed a temporary copy of your branch (but rebased) to → https://github.com/alphagov/govuk-frontend/tree/task-list-component which gets Heroku up and running again

@colinrotherham done. I've done a merge commit from main instead of a rebase for the time being, to avoid having to force-push and cause complications for anyone else who's checked out the branch.  We can rebase and squash the commits when we’re ready to merge. That ok?
I can't actually see where the "Allow edits and access to secrets by maintainers" checkbox is...?

Afternoon 👋
Just a heads up. We've moved our source code into npm workspaces package directories via https://github.com/alphagov/govuk-frontend/commit/102601ec1d7a7294b601e7f0c1141e785c0207dd
src → packages/govuk-frontend/src
app/src → packages/govuk-frontend-review/src
Hopefully it's not too much of a tricky git rebase but let us know if you need a hand
Publishing a pre-release or running npm link
If you're linking govuk-frontend locally into another project (or git filter-branch) also note:
package → packages/govuk-frontend-review/dist

Now that the Tag design changes have been merged into main, I’ve rebased this branch from main, so it now uses the new Tag design. cc @owenatgov @colinrotherham
Is there anything left to do?

Thanks @frankieroberto
I've added some comments and @owenatgov's already picked this up

I ran a quick test against VoiceOver and found that the aria-describedby setup wasn't working at all. The screen reader would declare the list item number, that it was a link and then I would have to step down the accessibility tree to hear the status and the hint, both the latter announced without any context.
I think this is fine on the surface as context is provided by the fact the user is within a list item, although I can't work out why describedby isn't working. I'll investigate this more. Generally I'm not 100% sold on using describedby here, mainly because I'm fairly confident you can't plug more than one id into it and it means the content of the thing being describedby is lost. As said above, I feel like letting the user step through the content of the list item is fine, but I'll verify that with the team.

Thanks for testing this. I think we mostly adopted the aria-describedby setup from the existing code in the prototyping kit - does it work ok there? I think the only thing we added was the option for the hint text.
The ARIA specification seems to suggest that multiple IDs are allowed but maybe that isn’t supported in practice?

So, after conferring with the team and re-testing, I was mistaken 🙈
describedby isn't actually intended to be announced when navigating using the arrow keys and only activates when navigating through sequentially focusable items on a page aka; navigating with the tab key. The basis for this is that you can get the context of the link being described by using the arrow keys to navigate to the surrounding content, which you should include nearby the link itself (which we do). The describedby is for when users are tabbing through links on a page and will therefore possibly miss the contextual content which we provide in describedby.
Re-testing with this in mind against NVDA and JAWS as well as VoiceOver, everything works as expected 🎉 I'm therefore happy to call this little issue resolved.

@owenatgov I’ve updated the param documentation, which was well out-of-date. Would you mind taking another look? I couldn't see any easy way to preview how this documentation would appear within the Design System website itself - I guess that'd need another pre-release?
I also updated the template to allow custom classes to be added at a few different levels: the
, the
 and the  or
 containing the title text. They can also be added to the tag and the status container. The only elements which don’t currently accept custom classes are
 (container for the title + hint) and the hint element (
). Do you think those ought to support custom classes too, or is that too much?

@owenatgov:

A few more comments. Most of them fairly petty code hygiene things. We're very close.

Thanks - fixed them all I think, apart from discussion around how to set the IDs.  Let me know if you spot anything else!

@36degrees thanks for spotting all these! I'll get onto them tomorrow!

@36degrees I think I’ve made all the changes - thanks for spotting them all! 👏
Would you mind approving the test workflows, and checking and re-reviewing the code? Thanks!

It'd be good to add some more tests to cover the various different macro options to ensure they're working as intended. This is a little laborious, so I'm happy to create an issue for our team to do this in a follow up PR unless you're particularly keen to be involved?

Discussed this with @frankieroberto and have raised an issue for us to do this (#3828).
I believe the other points have been addressed. This just needs a second review and then we can get it merged – @owenatgov would you be happy to take another look?


Resource artifactory_remote should support remote_repo_layout_ref attribute

Is your feature request related to a problem? Please describe.
I want to use the resource artifactory_remote_vcs_repository to manage github repositories.
To have a "pretty" internal layout, I want to have a different layout between  repo_layout_ref and remote_repo_layout_ref.
There is a comment for remote_repo_layout_ref attribute in documentation:
Deprecated field. This field has currently no effect, because there is no corresponding field in the API body, and it's not returned by the GET call.
Describe the solution you'd like
The field must have an effect on the generated remote repository.
API body must have this field.
Describe alternatives you've considered
Actually I can change the remote layout manually with artifactory ui but it can't be a long term solution
Regards

@Xaviou1003, thank you for the ticket. We can't fix that on the Provider side, but we have an internal ticket for the Artifactory team to fix that. Currently, we don't have any ETA.

@Xaviou1003, @alexhung I see that remote_repo_layout_ref bug is fixed in Artifactory 7.47.0. Once it's released, we need to test and remove the deprecation notice.


Add share path mapper with feature flag in the config

https://github.com/sciencemesh/cs3api4lab/issues/37 section 2

Please rebase a add some description of what is being done in the PR. It's a bit confusing to understand at first sight.

Please rebase a add some description of what is being done in the PR. It's a bit confusing to understand at first sight.

Code in this PR cover the share path from IOP and transform into the normal relative path. If a user has the share path eg. "/reva/einsten/share_folder/share_test.txt" and on "Share by me" window click on the "/reva/ path can see other user data. (issue on IOP; https://github.com/cs3org/reva/issues/1390).
The Mapping (share_mapper.py) creates an internal dictionary to store the original path and return to the frontend dictionary model with the mapped path. At reading containers or files it's executed the reverse process. If reading the path exists in a dictionary as the key it's replaced by the key-value and send to IOP instead of the path from the frontend.

Code in this PR cover the share path from IOP and transform into the normal relative path. If a user has the share path eg. "/reva/einsten/share_folder/share_test.txt" and on "Share by me" window click on the "/reva/ path can see other user data. (issue on IOP; https://github.com/cs3org/reva/issues/1390).
The Mapping (share_mapper.py) creates an internal dictionary to store the original path and return to the frontend dictionary model with the mapped path. At reading containers or files it's executed the reverse process. If reading the path exists in a dictionary as the key it's replaced by the key-value and send to IOP instead of the path from the frontend.

I still don't fully understand. Are you trying to avoid users from being able to click in the path that is gerated to access a share?
And I don't understand what the bug you found has to do with our lab extension. Once this gets fixed there's no problem for us, right?
Also if the shares are supposed to be accessible via our "Share" tab, we shouldn't expose "/reva/". We should only see there "Shares//". This way the user will only be able to return to the shares dialog and nowhere else. (I said from the begining that we will have to change the breadcrumb eventually.)
And I definetely don't understand why you are putting an in memory cache. This shouldn't be needed for anything as the mappings you need to do should be done based on the share id and nothing else (meaning that the mappings are static).

At some point in the future I want us to discuss exposing the full namespace (since is what we will be doing at CERN with CERNBox). Meaning that the user will start (home) in the path "/eos/user/d/diogo" and the shares might point to this path as well. So take that in consideration.

I still don't fully understand. Are you trying to avoid users from being able to click in the path that is gerated to access a share?
And I don't understand what the bug you found has to do with our lab extension. Once this gets fixed there's no problem for us, right?
Also if the shares are supposed to be accessible via our "Share" tab, we shouldn't expose "/reva/". We should only see there "Shares//". This way the user will only be able to return to the shares dialog and nowhere else. (I said from the begining that we will have to change the breadcrumb eventually.)
And I definetely don't understand why you are putting an in memory cache. This shouldn't be needed for anything as the mappings you need to do should be done based on the share id and nothing else (meaning that the mappings are static).

At some point in the future I want us to discuss exposing the full namespace (since is what we will be doing at CERN with CERNBox). Meaning that the user will start (home) in the path "/eos/user/d/diogo" and the shares might point to this path as well. So take that in consideration.

I think we are able to freeze and close this PR now and will in the future, because:

you are going to change the EOS names structure's
yesterday presentation about changes in EOS
I spoke with Hugo on WP3 meet about this PR (https://github.com/cs3org/reva/issues/1390) and this bug should not have happened on a production system, because EOS watches over file access

@diocas are you agree?

I think we are able to freeze and close this PR now and will in the future, because:

you are going to change the EOS names structure's
yesterday presentation about changes in EOS
I spoke with Hugo on WP3 meet about this PR (https://github.com/cs3org/reva/issues/1390) and this bug should not have happened on a production system, because EOS watches over file access

@diocas are you agree?

Fine by me. But keep in mind that this extension needs to work in deployments without EOS as well. But the access control to other files will for sure be imposed in wtv storage backend is used.

Fine by me. But keep in mind that this extension needs to work in deployments without EOS as well. But the access control to other files will for sure be imposed in wtv storage backend is used.


fix: falcoctl installation command (for linux amd64 system)

What type of PR is this?

/kind content

Any specific area of the project related to this PR?

/area blog

What this PR does / why we need it:
The actual command extract the falcoctl binary but also LICENSE and README files. So if you already have a README and a LICENSE file in the current folder, they are replaced with the falcoctl ones ;) (yes it happened to me ^^).
So I fixed the command to extract only falcoctl binary.

[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: scraly
Once this PR has been reviewed and has the lgtm label, please assign vjjmiras for approval. For more information see the Kubernetes Code Review Process.
The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

content/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Welcome @scraly! It looks like this is your first PR to falcosecurity/falco-website 🎉

[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: Issif, scraly
The full list of commands accepted by this bot can be found here.
The pull request process is described here

Needs approval from an approver in each of these files:

content/OWNERS [Issif]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

LGTM label has been added.  Git tree hash: 7033753f52c52b50b85db5c701ecf714a8b18a2b


[Bug]: Unknown address auto-connected on xdai using Gnosis Safe

Current Behavior
Loading https://xdai.curve.fi through a Gnosis Safe auto-connects an unknown address. Reconnecting manually by triggering the Onboard modal again and picking "Gnosis Safe" connects the right Safe address.

Expected Behavior
Loading https://xdai.curve.fi  through a Gnosis Safe should auto-connect to the Safe's address; note: on curve.fi's onboard integration, on first load there won't be any auto-connecting, users do need to pick "gnosis safe" in the Onboard modal; this is different behavior from the described "current behavior" which auto-selects a seemingly random address
Steps To Reproduce
No response
Onboard Version
<EMAIL_ADDRESS><EMAIL_ADDRESS>Node Version
v12.22.7
What browsers are you seeing the problem on?
No response
Relevant log output
No response
Anything else?
I haven't had the ability to dive deeper into this issue to make sure it's 100% one of those packages exhibiting an unexpected behavior. However, loading Curve.fi sites like curve.fi (Ethereum) or polygon.curve.fi (Polygon) using Gnosis Safe doesn't exhibit the behavior described in "current behavior": they behave like "expected behavior": this alone would seem to point to @web3-onboard/gnosis

@philippe-git is this issue still happening?

@philippe-git closing this - if this issue re-occurs feel free to reach back out


Remove font-smoothing

This is a nonstandard CSS feature and is not supported on many platforms. And has been removed from the CSS standard specification.
It is advised to not use it according to MDN along with many other guides on the internet.
https://developer.mozilla.org/en-US/docs/Web/CSS/font-smooth

Some argue that this is still worth it on darker backgrounds but not sure if it is worth it. And could cause more problems then it fixes. If we want to keep this we will need to create a custom mixin if we want to remove bourbon.

This looks good! font-smoothing should be removed.
As for the dark backgrounds, this argument from responsivedesign.is/articles/font-smoothing makes a good point:

If you’re design is placing text onto a dark background then you have a good reason to look towards using the WebKit and Firefox prefixed font-smoothing options to make the text look lighter, but you should be warned that these only work on Mac and OSX and leaves the billions of other users with a sub-standard view.
What should I do?
I recommend not using it, and instead if you’re looking to get a lighter version of the font then use the font-weight to control this so that it appears nicely across both Mac and Windows, and works across all browsers without the need of a browser prefix.

Further reading: stop-fixing-font-smoothing from usabilitypost.com.


Getting error for BLPOP command

Hi,
Getting below error for BLPOP command.
`20:39:00.387 [main] INFO  org.ketteq.poc.redis.Redis - Command: [LPOP, KEY-1, 1000]
org.ketteq.poc.redis.protocol.Parser$ServerError: ERR Protocol error: expected '$', got ':'`
Method used:
 Redis r = new Redis(new Socket("<IP_ADDRESS>", 6379));         Object output = r.call("BLPOP", "KEY-1");
Please let me know what exactly wrong I am doing here.

Hi, for some reason this message totally escaped my attention, sorry :) Did you figure it out, or do you need more help?


Update for Atom >= 1.13

Updates to make Deprecation Cop in Atom happy.  I'm a total n00b at css/less, so please let me know if I did something dumb.

This is pretty much the same as #3 , either would be good to pull in.


Cant get settings file to work

I've tried to follow the documentation for creating a settings file but it won't work. I added a settings.ini in the config folder but it seems like it's ignored. Can you clarify a bit more how it should be done?

are you referring to the config folder within reviews/app/config - if so, you may want to locate your configuration outside of the codebase (as the settings.ini will only be picked up from the root of the project - in case you also wish to install directly from PyPi, i'd recommend setting the PATH_TO_CONFIG and locate it under ~/home/.config/reviews
The following are the steps I used to test the configuration file. Have a look at these and let me know if it solves this issue for you.: https://github.com/apoclyps/reviews/pull/143 . Multiple options are present in the description however I think you might be aiming to use either Testing .ini file within the repository or Testing using PATH_TO_CONFIG.

@bphermansson were you able to get the configuration working? If not, please let me know what issue you encountered and I can update the documentation to help others.


Field: Argument 'title' has incorrect type when passing a lazy proxy. Cythonized

Checks

[ x] I added a descriptive title to this issue
[x ] I have searched (google, github) for similar issues and couldn't find anything
[ x] I have read and followed the docs and still think this is a bug

Bug
Output of python -c "import pydantic.utils; print(pydantic.utils.version_info())":
             pydantic version: 1.7.1
            pydantic compiled: True
                 install path: /home/me/venv/lib/python3.8/site-packages/pydantic
               python version: 3.8.5 (default, Jul 28 2020, 12:59:40)  [GCC 9.3.0]
                     platform: Linux-5.4.0-54-generic-x86_64-with-glibc2.29
     optional deps. installed: ['typing-extensions']

Trying to pass a proxy object into title typed as str.
from django.utils.translation import gettext_lazy as _

from pydantic import BaseModel
from pydantic.fields import Field

class MySchema(BaseModel):

    alias: str = Field(title=_('Alias'))

This worked on py3.6 (Ubuntu 18.04) for a long time but failed in new env with py3.8 (Ubuntu 20.04) giving:
TypeError: Argument 'title' has incorrect type (expected str, got __proxy__)
This seems basically the same as mentioned in #1070: dropping fields.cpython-38-x86_64-linux-gnu.so fixes that.

Is there any solution to this problem?

Is there any solution to this problem?

It seems that so far deleting fields.cpython-38-x86_64-linux-gnu.so from package installed is the only way.

Given the issue is two years old, and there wasn't much interaction, and also considering that we are focusing on releasing V2, I'll be closing this issue.
We don't cythonize Pydantic on V2, so we are good.


More pirate weapon content(two new cannon ammo types, a musket and flintlock resprite and rebalance)

About the PR

I added two new cannon ammo types(grapeshot and glassshot), gave the musket and flintlock a resprite and rebalance.
Why / Balance

More pirate content, I'll probably make a PR making these things available to the crew in some way.
Technical details

Media

[X] I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

:cl:

add: Added two new ammo types for the pirate cannon. Tally Ho!
tweak: Changed the musket and flintlock sprites, as well as some functionality.

Those cannon shots seem very powerful.
One thing you could do, but I don't really know if its better, is just fill the glass shot with the actual glass shard prototype from like broken lights or glasses and stuff. Anything that does throw damage will do damage out of a gun.
Does this bayonet on the musket work? I thought the reason it wasn't on the mosin was it no worky.

Does this make it always shoot 30 degrees off to the left or right?

I like the flintlock sprite, its a big improvement.
Those cannon shots seem very powerful.
One thing you could do, but I don't really know if its better, is just fill the glass shot with the actual glass shard prototype from like broken lights or glasses and stuff. Anything that does throw damage will do damage out of a gun.

I wanted to have the shards stick into enemies for a bit

By the way, the use cooldown on the musket straight up doesn't work for guns after testing in game.


Upgrading to 0.11.3

Issue Summary
I'm upgrading my ghost hosted on digital ocean. When I run npm start --production, I get this error:

Browser/OS: chrome

I'm using their 1 click deploy image here: https://www.digitalocean.com/products/one-click-apps/ghost/

Hi @krumjahn!
We're sorry to hear you're having problems, but ask that you do not use our GitHub repo for support. Please read our support guide. You can always use our slack community - https://slack.ghost.org, if you're not able to find the answer in our documentation - https://docs.ghost.org.


t.Tranlate(x,y) is not wrong

your vector translation comes down to
func MultiplyTransforms(a Transform, b Transform) Transform {
    var t Transform
    for i := 0; i < 3; i++ {
        for j := 0; j < 3; j++ {
            t[i][j] = t[i][j] + a[i][j]*b[j][i]
        }
    }
    fmt.Println(b)
    fmt.Println(t)
    return t
}

i am not a 100 percent sure, but i think you don't multiply but add for a translation

I think I have fixed the matrix multiply function finally, which should fix the translate.


RuntimeError: Output 0 of ReshapeAliasBackward0 is a view and is being modified inplace.

Hi,
Thanks for your work! I encountered an issue when running the command to train a model with your methods:

python ./tools/train.py ./configs/ours/ganav_group6_rugd.py

python ./tools/train.py ./configs/ours/ganav_group6_rellis.py

Both of the commands prompt this error:

I am still new to this topic, could you kindly let me know how to solve this problem?

Hi,
I have never seen this issue before. I want to make sure I have all the packages version correct.
In addition, I found this website that might help you resolve the issue: https://stackoverflow.com/questions/72498093/when-using-attention-augmented-convolution-layer-from-source-code-in-resunet-g#:~:text=RuntimeError%3A Output 0 of ReshapeAliasBackward0,out-of-place one.
Those scripts essentially process the data, and the logic is straightforward. In worst case, you can modify the line that gives your the error as long as you make sure the logic is not changed.

I have never seen this issue before. Please make sure you have the correct version of the packages.
In addition, I found this website that might resolve your issue: https://stackoverflow.com/questions/72498093/when-using-attention-augmented-convolution-layer-from-source-code-in-resunet-g#:~:text=RuntimeError%3A Output 0 of ReshapeAliasBackward0,out-of-place one.
Could you give more details on the error? Which line caused it and the entire error message would be helpful.

Thanks! In fact you only need to show the last image, which shows error and which line has the error.
It seems that in transnet.py, line 179, you have a in-place operator. I can not reproduce this error, but you can simply change it to a out-of-place multiplication operator.

Bizarrely had this issue as well, all it took to fix it was to change
q *= self.scale

to
q = q*self.scale

Thank you for the fix -- I will update this change.


[Subtask] HiveEngineEvent toString is not pretty

Code of Conduct

[X] I agree to follow this project's Code of Conduct

Search before asking

[X] I have searched in the issues and found no similar issues.

Describe the subtask
2022-04-08T11:34:33,368 INFO [main] org.apache.kyuubi.engine.hive.HiveSQLEngine - org.apache.kyuubi.engine.hive.events.HiveEngineEvent@ec865c63
Parent issue
#2010
Are you willing to submit PR?

[ ] Yes I am willing to submit a PR!

please assign to me.😁

assigned @wolfboys ,thanks


Not able to render Font Family while conversion to PNG & JPG

Description
I added Font Face to SVG, Font family shows in SVG Preview, but when I convert SVG to Bitmap using Draw method, then save the Bitmap to PNG, JPG. It does not render the font.
NOTE: The font shows on SVG but not in PNG, JPG
Example data
Code in SVG:

Code in C#
Bitmap bmp = mySvg.Draw(pixelWidth, pixelHeight);
<EMAIL_ADDRESS>jgpEncoder,myEncoderParameters);
Used Versions

SVG version used: 3.4.4
Please help me out, I am stuck at the end of deadline, all my work is complete for image generation, only the Font family is not showing. Thanks in advance.

Duplicate of #1060.

I wish to get a reply ASAP, hence the duplicate.
On Fri, 26 May 2023 at 4:00 PM, mrbean-bremen @.***>
wrote:

Closed #1059 https://github.com/svg-net/SVG/issues/1059 as completed.
—
Reply to this email directly, view it on GitHub
https://github.com/svg-net/SVG/issues/1059#event-9349198827, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/BADTUGCA4U3SMJ5DMQRKBMLXICA5FANCNFSM6AAAAAAYP6TYMY
.
You are receiving this because you authored the thread.Message ID:
@.***>

Duplicates don't get you an answer faster.
I see that you are new to GitHub - you have to understand that the people contributing and answering here do this in their free time, and getting some ASAP message or double-posting it usually will not help - on the contrary, most people will may get annoyed.

Understood, thanks.
Actually I posted one as a “New Issue” and second one in QnA section.
Won’t happen again. Thanks for the heads up.
On Fri, 26 May 2023 at 4:35 PM, mrbean-bremen @.***>
wrote:

Duplicates don't get you an answer faster.
I see that you are new to GitHub - you have to understand that the people
contributing and answering here do this in their free time, and getting
some ASAP message or double-posting it usually will not help - on the
contrary, most people will may get annoyed.
—
Reply to this email directly, view it on GitHub
https://github.com/svg-net/SVG/issues/1059#issuecomment-1564223405, or
unsubscribe
https://github.com/notifications/unsubscribe-auth/BADTUGBDAILF4XHJQIBA2FTXICE63ANCNFSM6AAAAAAYP6TYMY
.
You are receiving this because you authored the thread.Message ID:
@.***>


Rubocop-like JSON output

Hi!
I would like ask that are you planning to add an optional rubocop-like json output format?
We have a bot, which checks pull requests with rubocop.
I would like to add support for the haml files with haml-lint, but I need JSON as output.
Thanks!

Hey @skateman,
No plans, but would happily accept a pull request!

That's what i wanted to hear ;) I'm on it!

Implemented by #63


Merging to master will no longer throw errors on test

This comments out the deployment code that was breaking the build on master.  This is not necessarily an optimal solution though.  In theory this should work given the current deployment scripts there are at least two obstacles that I know of to getting this working, one may not be readily solvable

(easy to solve) The Clusternator project needs a clusternator project create-data run on it to generate the relevant keys which (I really hope!) contains the .private/clusternator-aws.json stuff that future stories will eventually replace
(maybe not readily solvable) Can we use environment variables on public GitHub/CircleCI projects and is it safe?

@rafkhan what do you think?

Using env vars on a public repo is a pretty bad idea on circle

:shipit:


fix templateUrl regex

Codecov Report

Merging #51 into master will not impact coverage.

@@           Coverage Diff           @@
##           master      #51   +/-   ##
=======================================
  Coverage   96.42%   96.42%
=======================================
  Files           1        1
  Lines          28       28
  Branches        5        5
=======================================
  Hits           27       27
  Misses          1        1

Impacted Files
Coverage Δ

index.js
96.42% <100%> (ø)
:white_check_mark:

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 dc5de16...8a61ec4. Read the comment docs.

Hi, I think, it doesn't handle cases when templateUrl line is ending by space character. I created PR https://github.com/TheLarkInn/angular2-template-loader/pull/52 to resolve this issue.

@maksim-tolo
yep, I missed it.
Thank you.

Closed by https://github.com/TheLarkInn/angular2-template-loader/pull/52


[RFC] Clean up Logger saving of Profiler output

Background
Currently we write our checkpoint and profiler output to the Logger, in the case that the user does not specify a path.
(Note: this is for the case of one logger, the case of multiple loggers to be discussed in a different issue).
The exact priority followed is:
Profiler:

if a dirpath is provided write to it
if not, write to the Logger
if there is no Logger, write to default_root_dir

Checkpointing:

if a dirpath is provided write to it
if not, and a weights_save_path is provided in the Trainer, write to it (Deprecated in https://github.com/PyTorchLightning/pytorch-lightning/issues/11768)
if not, write to the Logger
if there is no Logger, write to default_root_dir

Motivation
There are several issues and inconsistencies with how we currently write to the Logger.

One issue is that for the checkpoint output we put it in a nice directory called "checkpoints", but for the profiler output it is not in a nice directory called "profiler" but instead the individual output files are just sitting in one of the logger's directories.

There are inconsistencies in terms of where the loggers store the profiler output (in the case where no dirpath is provided). Most of the loggers store the output in their save_dir, but the TensorBoardLogger stores the profiler output in the same place where it stores the checkpoints - save_dir/name/version.
https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/trainer/trainer.py#L2095-L2107

Why do we have different behavior for the TensorBoardLogger? Also, why is the profiler output not in the same place as the checkpointing output?
Pitch

When logging to the logger, put the profiler output in a directory called "profiler".

All loggers should store their output in save_dir/name/version. Introduce a new property on the Logger Base API called log_dir (TBLogger and CSVLogger already have it), which returns this path, and is the place to store profiler and checkpointing output.

This will allow us to clean up this code here:
https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/trainer/trainer.py#L2095-L2107
And also massively simplify this code:
https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/callbacks/model_checkpoint.py#L582-L596
replacing it with simply ckpt_path = os.path.join(trainer.log_dir, "checkpoints") after https://github.com/PyTorchLightning/pytorch-lightning/issues/11768 is also complete.
cc @awaelchli @edward-io @borda @ananthsub @rohitgr7 @kamil-kaczmarek @Raalsky @Blaizzy @ninginthecloud @carmocca @kaushikb11

I tried to do this once too, standardizing the log dir, without success #7543. We can try again :)

I've been struggling with the situation where trainer.log_dir doesn't make sense when using other loggers and can only use TensorBoardLogger. For instance, SaveConfigCallback saves the configuration under trainer.log_dir, but for different runs, default_root_dir and logger.save_dir are usually the same, which cause saving conflict, as I raised in the previous issue https://github.com/PyTorchLightning/pytorch-lightning/issues/7542 which @awaelchli failed to fix in https://github.com/PyTorchLightning/pytorch-lightning/pull/7543.
https://github.com/PyTorchLightning/pytorch-lightning/blob/7d1075d2116f8ceda9f7bf12cc083fbc1eb0f32e/pytorch_lightning/trainer/trainer.py#L2202-L2213
I like the idea that use save_dir/name/version as log_dir on the Logger Base, but I found a lot of difficulties when I tried to implement it in practice. The following are some of the troubles I encountered for reference. I have only looked at wandb and neptune, which may be more complicated when considering other loggers.

The name and version of NeptuneLogger are None at setup when ModelCheckpoint __resolve_ckpt_dir because the actual neptune run object is initialize later than setup.
save_dir, name, and version are not always can be found in third-party loggers, or cannot be modified by the user.

User cannot customize Neptune version currently, see https://github.com/neptune-ai/neptune-client/issues/876
Wandb hardcodes its run_dir https://github.com/wandb/client/issues/1620 https://github.com/wandb/client/blob/5dc9a15a3f666005a1f726e9430a178da2151463/wandb/sdk/wandb_settings.py#L618 and neptune hardcodes its save_dir, which causes the use of these loggers to store multiple directories, one for the PL and one for the Loggers themselves.

If save_dir, name, and version are not required, there should be a uniform policy for generating them, but each logger may be different, see 2

@awaelchli @daniellepintz   Considering the above complex situation, I have a proposal:

Every logger should have save_dir, name, version as init_args with default values for PL own uses (e.g., saving checkpoints, profiler outputs, and cli config). The default value of them are : str = './', : str = 'lightning_logs' and : Optional[str] = None, respectively.
We can use the incremented number as the generation strategy for version, as TensorBoardLogger does, if users don't specified. Or use timestamp.
We just need to make sure that the third party logger stores its own logs in save_dir (in case the script directory is not available after the experiment), because they all have their own directory format, and it is impossible to make them follow save_dir/name/version see https://github.com/PyTorchLightning/pytorch-lightning/issues/12036 https://github.com/wandb/client/blob/5dc9a15a3f666005a1f726e9430a178da2151463/wandb/sdk/wandb_settings.py#L618.
If possible, we should map save_dir, name and version to the logger initialization parameters (for example, dir, name and id in wandb init), and if not (like netpune), we should also log these three values to the loggers.
These three properties of loggers should return their own values, not third-party values, especially if they cannot be mapped in 4.
In this case, it is natural to make the dir, name and version of multiple loggers consistent and there is no question of where to store them.
For none logger case?

Hi @tshu-w thanks for your analysis here! Yes, these are all things I am aware of and planning to address as part of this issue. No need to raise a new RFC since the proposal here covers it :)

Cool, looking forward to seeing the finalized plan, so I can help modify the Loggers.

@carmocca @daniellepintz @awaelchli Hi, We can see that there are more and more issues #12748 #14054 related to loggers and can be solved by my proposal https://github.com/Lightning-AI/lightning/issues/12028#issuecomment-1088325894. Would you like to discuss it and then I'll be happy to help implement it in 1.8.

My advice is, in all of your considerations, please take into account multiprocessing and the requirements around them, which are mainly two:

No code changes should be required by the user when switching accelerator, strategy, number of devices
Loggers need to remain pickle friendly

Most of the other limitations and design choices we have today are a direct consequence of multiprocessing challenges.
In your comment:

This sounds great. But needs to be verified for each logger that this is feasible.
This can be problematic when versioning is done differently in the 3rd party. You will end up with two versions for the same experiment. Please take into account that versions are also used to resume experiments.
This is a big challenge I have given up a long time ago. Feel free to give it a try :)) I would love to see this being addressed.
This may not apply to all loggers. This is difficult to do.
This goes back to my concern in 2)
I don't understand what this is saying

I'm not sure, but I think @daniellepintz is not working on this anymore. Feel free to explore, I remain skeptical and curious at the same time :)

No code changes should be required by the user when switching accelerator, strategy, number of devices
Loggers need to remain pickle friendly

Agreed, may need someone to confirm for me.
Regarding your concerns,

2. This can be problematic when versioning is done differently in the 3rd party. You will end up with two versions for the same experiment. Please take into account that versions are also used to resume experiments.

This is the trade-off I mentioned earlier in issue 1:

The name and version of NeptuneLogger are None at setup when ModelCheckpoint __resolve_ckpt_dir because the actual neptune run object is initialize later than setup.

We cannot get Logger version at setup but we need it to __resolve_ckpt_dir, so we had to have our own version. Is there any problem with initializing the third-party logger instance in PL logger __init__ ?

4. This may not apply to all loggers. This is difficult to do.

Yes, but if we can't(like netpune), we should also log these three values to the loggers.

5. This goes back to my concern in 2)
Users can find the version on the dashboard, or use pl_logger.experiment.get_XXX() to get actual third-party logger object.

6 and 7 are for no logger or multi loggers.

default_root_dir is not a good place to save all things as there will be conflicts
I think save_dir, name, and version should be used as trainer parameters because even without a logger, you need to save things under save_dir/name/version (checkpoint, profiles, and others).