Choose a more specific name than "signing-spec"

"signing-spec" is a generic working title for this repo/project/specification. It should be replaced by something more specific.
Side-goal courtesy of @MarkLodato: Ideally the new name is easy to search for.
See #1 - What is secure-systems-lab/signing-spec? for context.

It's probably past time to kickstart this. Does anyone have any thoughts on what we can name this spec? Any initial leads we could iterate over?

Some random thoughts:
JSON Artifact Envelope - JAE - Not great because it's not actually an envelope for artifacts.
involucrum - Latin for envelope.  Using Latin to play off of in-toto. Probably pretty awkward to say.
JSON Signed Envelope - JSE
in-toto Attestation Envelope - IAE (maybe we don't want to reference in-toto?)

More ideas:

"envelope" or "signature" in another language.
"Foolproof Signed Envelope"
"Dead Simple Signed Envelope", pronounced "dissy"

I like Dead Simple Signed Envelope! I was considering options along the lines of "Lightweight", but I think this may be crisper.

Another option that is more descriptive:

"Signed Type-Identified Message" (STIM)
"Signed Message with Authenticated Type" (SMAT)
or similar

If we think about the advantages of this spec over others, the main thing is that it authenticates the type in addition to the message. The only other spec that provides that is JWS (via typ), but that spec is super complicated and error-prone.

Type Authenticated Cryptographic Object - TACO
🌮

Oh man, I love it! Too bad the name is overly broad!

tamale: Type And Message Authenticated in a Lightweight Envelope

I vote DSSE

I have a slight preference for tamale but am fine with DSSE.
Any more votes?
I suggest that we go with DSSE unless we hear any dissenting opinions within the next few days.

I'm fine with either.
On Fri, Jun 4, 2021 at 11:18 AM Mark Lodato @.***>
wrote:

I have a slight preference for tamale but am fine with DSSE.
Any more votes?
I suggest that we go with DSSE unless we hear any dissenting opinions
within the next few days.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/secure-systems-lab/signing-spec/issues/16#issuecomment-854807222,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABHZSMEW6NY3FLAEZHD3UWDTRDVDLANCNFSM4XCSYKSA
.

I'm fine with either as well.


Running podium on Linux platform

Hi @freakboy3742, @danyeaw
I was working one day at PyCon sprints on trying to make podium run on Linux platforms.
The issue was that application exits immediately after starting it without launching application window and with no errors.
>>> from podium.app import Podium
>>> papp = Podium()
>>> papp.main_loop() # print statements before and after self.loop.run_forever(application=self.native)
self.loop:    <GtkEventLoop running=False closed=False debug=True>
self.loop:    <GtkEventLoop running=False closed=False debug=True>

When we execute papp.main_loop() method we should see only the first print statement and podium app window should pop up.
Instead no window pops up and self.loop.run_forever() exits out.
I think I found the reason - the podium app is not setting the interface correctly. For example:

For minimal app:

>>> import toga
>>> app = toga.App('Empty App', 'org.beeware.empty')
>>> app._impl
<toga_gtk.app.App object at 0x7fb9d9f17ba8>
>>> app._impl.interface
<toga.app.App object at 0x7fb9da7a5710>

For podium app:

>>> from podium.app import Podium
>>> papp = Podium()
>>> papp._impl
<toga_gtk.app.DocumentApp object at 0x7fb9d1cb3eb8>
>>> papp._impl.interface
<podium.app.Podium object at 0x7fb9d1cb38d0>

As you can see the podium app interface is itself - podium app. I think it should be higher level toga.app.DocumentApp interface.
As a result when self.interface.startup() method gets called it only calls the podium.app.Podium.startup() method which only sets the Podium application commands and  does not call the higher level interface startup() method which supposed to create and launch main window application.
Can you comment if this is correct?
Thanks!

@andreysmelter I don't think that's right. podium.app.Podium is a subclass of DocumentApp, so the interface that papp._impl is correct, AFAICT.
The problem (as I understand it, anyway), is in the nature of a DocumentApp itself. In a base App, starting an app creates a Window. However, in a DocumentApp, The App starts, but only creates a Window when it opens a Document. It might do that because the App was opened with a double click on a document icon; but if the app was opened without a reference to a document, it still needs to start, and direct the user to an "open file" dialog.
However, because the application is being created without a window, the startup logic isn't being invoked correctly. I don't know enough about GTK to comment on why that is, but "starting an application without a window" is somewhat counterintuitive behavior, so it's not entirely surprising.

The issue was that application exits immediately after starting it without launching application window and with no errors.

This seems to describe the behavior mentioned in https://github.com/beeware/podium/issues/11#issuecomment-515740679.

The Hello World tutorial app has a show() call in its startup code, which I can't spot in podium.app:
class HelloWorld(toga.App):

    def startup(self):
        self.main_window = toga.MainWindow(title=self.name)
        self.main_window.content = toga.Box()
        self.main_window.show()

GNOME's GTK4 docs suggest that there must be a window shown for a GTK application to be launched. Maybe the app's main loop is tied to that logic?
Should we investigate whether it's possible to launch a GTK app without a window? Or abort with an error or something?

@bittner The underlying issue here is that Podium is a "document based" app - that is, the windows in the app relate to open documents, rather than "the app" itself. A word processor is the classic "document-based app" - there's no idea of opening the word processor app without opening at least an empty document.
A "simple" app (like HelloWorld) has a main window, and thus a "show" call to display that window. It can also have other windows, if it wants them; but it's up to the app to manage them.
A document-based app doesn't have a main window - it has a collection of documents, each of which has one (or more) windows.
That explains why there's no show() call in the Podium  - the app doesn't need to show anything. But each document that the app opens does - and if you follow the breadcrumbs down the Document definition for a slide deck, you'll see that opening a single SlideDeck Document results in 2 windows being created, both of which are shown as a part of opening the document.
Toga has an API for document-based apps, but that API has only been implemented (so far) for macOS. That's one of the two big pieces that is missing before Podium will work (the other is implementing the full-screen API).
On macOS, there's no requirement that an app have a window. However, if an app starts and there's no document specified, the app will automatically open a file dialog. I'd suggest that the GTK approach would be to emulate this sort of behavior. If you start the app and a document is provided (e.g., by double clicking on a document, or as a command line argument), then that document is opened; however, if there's no document specified, then the application will open a file dialog. If the user closes an open document, and ever ends up in a "no documents open" state, then it reverts to displaying the file open dialog. That way there will always be an open window - even if that window is just a dialog.
(That said - I will also state that I'm not a Linux native user, so I'm not necessarily 100% familiar with Linux desktop idiom. The fundamental aim of all Toga APIs is to generate behaviors that result in an app that feels completely native, not "macOS behaviors crammed into a Linux shell". The concept of an app that operates on a document is universal; the behaviors around that aren't necessarily universal. So - no matter what I've said here - the actual behavior we should be aiming for is "whatever a truly Linux native app would do". So - what happens when you start OpenOffice? Do that)

Fixes for toga: https://github.com/beeware/toga/pull/714
Fixes for podium: #31

Closed by #31.


Could not find io.insert-koin:koin-annotations:1.0.0-beta-1.

Hi, this looks promising, I'd love to define singletons and factories just by annotating the class, thus avoiding having to declare it in my module every time.
But when I try to run the sample app or add it to my project based on the guide provided I get the following error:

Could not resolve all files for configuration ':getting-started-koin-android:app:debugRuntimeClasspath'.
Caused by: org.gradle.internal.resolve.ModuleVersionNotFoundException: Could not find io.insert-koin:koin-annotations:1.0.0-beta-1.

Do you know why this might be?
Thanks

Same thing

@stankinzl okay, it is work now for me. Try it again

still getting problem?


BUG - longtext_url property not being displayed in ABAP deployment error messages

Description
When prettyPrintError method is called, its not handling the longtext_url property correctly so no clickable URL is given to the user.
The missing URL link is for both deploy and undeploy ABAP flows.
Sample error message;
"errordetails": [{
         "code": "FINS_REV_REC/010",
         "message": "Maintain plan costs suitable for WBS-Element JLA-13 using revenue recognition method 'Recognize revenue on cost-based POC'",
         "longtext_url": "/sap/opu/odata/iwbep/message_text;o=SFINODATA1/T100_longtexts(MSGID='FINS_REV_REC',MSGNO='010',MESSAGE_V1='Recognize%20revenue%20on%20cost-based%20POC',MESSAGE_V2='WBS-Element%20JLA-13',MESSAGE_V3='',MESSAGE_V4='')/$value",
         "propertyref": "",
         "severity": "error",
         "target": ""
}]

Fiori Tools deployment handling the longtext_url property

Deploy-tooling, missing URL link

Steps to Reproduce
Steps to reproduce the behavior:

Generate and deploy an ABAP Fiori UI application
Deploy the application
Change the project name
Redeploy the application
Duplicate ID error is shown

Expected results
longtext_url link is generated and clickable.
Actual results
longtext_url not shown as part of the duplicate ID error response.
Screenshots
Applied above
Version/Components/Environment
Add any other context about the problem here
OS:

[x] Mac OS
[x] Windows
[ ] Other

Root Cause Analysis
Problem
{describe the problem}
Fix
{describe the fix}
Why was it missed
{Some explanation why this issue might have been missed during normal development/testing cycle}
How can we avoid this
{if we don’t want to see this type of issues anymore what we should do to prevent}

Validated locally within our application and working as expected.


Multiple MdDialog on IE11

Bug, feature request, or proposal:
On IE11 i'm unable to open an MdDialog inside another MdDialog.
On Chrome and Firefox it works properly.
What is the expected behavior?
Open an MdDialog inside another MdDialog
What is the current behavior?
Nothing happens.
Which versions of Angular, Material, OS, TypeScript, browsers are affected?
IE 11

Strange I thought I searched for IE 11 issues. Just close this one then.

The same issue happens also in Edge with the beta.10 release with beta.8 it worked.

Closing as a duplicate of https://github.com/angular/material2/issues/6719.


Adjust the puma server start script

Why

Note
Adjust the rails_docker/start.sh, https://github.com/nimblehq/rails-templates/blob/develop/shared/Procfile.dev, https://github.com/nimblehq/rails-templates/blob/develop/shared/Procfile
bundle exec rails s -p $PORT -b <IP_ADDRESS> -> bundle exec puma -p $PORT -b <IP_ADDRESS>

Rails integrated with Puma by default so that with the rails server it will call the config/puma.rb, and some other puma's feature we don't use it so we could keep going with rails s if a project needs to use and custom puma's feature then will change in that project.


[WIP] Fixed #24989 -- Added migrations API documentation

[skip ci]

Good start. Please reopen when it's ready for review.


PPE supplies and shortage per hospital in US

PPE shortage is coming with covid-19 patients' growth per hospital.
To answer the questions if we can model/simulate PPE (e.g. facial masks, surgical gloves, gowns, eye protections etc.) uses/shortages
We need to think through:

The medical procedure/treatment for a few group of covid-19 patients, e.g. critical care or not;
staffs numbers per patient, including doctors and nurses;
how many patients and how many doctors are working each day in each hospital, how frequently they need to use the PPE and other supplies. of course,
the availability of the PPE and all supplies.

Note: The above table cited from a peer-reviewed article: Personal protective equipment in an influenza pandemic: a UK simulation exercise (2007).
NYT article Governments and Companies Race to Make Masks Vital to Virus Fight

President Trump resists using emergency powers to compel production, saying companies will voluntarily provide much-needed protective gear.

Doctors and hospital administrators have been warning for weeks that they face acute shortages of masks and lifesaving equipment such as ventilators. State and local officials in New York and elsewhere have been pleading for help from Washington and the industry.

Thanks @Geoyi for starting this issue and working on this key part of the problem of protecting our healthcare workers!
Adding some thoughts that may help with considering what needs to be modeled to estimate PPE supply. This is mainly coming from talking through the problem w/ a clinician friend who sees inpatients, teaches residents, and needs to make decisions about conserving PPEs and minimizing exposure risk to them with every patient encounter:

the number of COVID Patient encounters per Provider per Day may be a good field to base other estimates on. This tries to capture how many times a Provider needs to don and doff PPEs in a day's work, tied to the number of COVID pts they see (which may vary widely depending on the role of the provider, more on this below).
This can then be used to represent the overall consumption rate of PPEs in a provider workforce. In ideal standard practice, this would be 1 set of PPEs per Pt encounter per Provider (since they're supposed to be single use) but in today's situation, providers are being asked to conserve and reuse. So this can be an adjustable estimate like 0.1 sets of PPEs per Pt encounter (representing 1 set of PPEs being used for 10 patient encounters before getting disposed and requiring a new set).
The number of encounters a provider has with COVID pts also depends on the role. For example, A critcare nurse in a dedicated COVID ICU ward would be exposed to many more COVID pt encounters per day than a specialist consultant and need many more PPE sets per day.
Staffing ratios for providers, nurses, techs, and anyone else needing to don PPEs with each patient encounter may become very dynamic and differ from usual practice. Health systems are putting in new policies to minimize provider exposure risk, usually by reducing the overall # of encounters between suspected/confirmed COVID pts and provider teams. For example in a teaching hospital, residents may no longer be seeing every patient in person ahead of the attending or there may be dedicated, smaller teams of 1 attending & 1 resident and the same for the nursing teams. This means the staffing ratios should be adjustable to account for this flux.

These are super helpful and thank you for talking through the problem with your clinician friend, @daveluo.

It's also worth at least keeping in mind for ourselves the various types of PPEs: N95 respirators, surgical/procedure masks, eye protection, gown and gloves
and how they correspond to the different usual levels of isolation precautions: Contact, Droplet, Airborne
N95 masks and face shields seem to be the most common and needed requirement now but we also see use of surgical/procedure masks, gloves & gowns during screening and triaging patients. Or procedure masks in lieu of N95 when there aren't any.

I am reading reports that many facilities are minimizing their usage to 1 PPE (mask) per day.

@andrewwgithub thanks for the input! To be more specific, do you mean one N95 per provider per day?

Also adding to thread as a reference: JAMA: Conserving Supply of Personal Protective Equipment—A Call for Ideas

The editors of JAMA recognize the challenges, concerns, and frustration about the shortage of personal protective equipment (PPE) that is affecting the care of patients and safety of health care workers in the US and around the world. We seek creative immediate solutions for how to maximize the use of PPE, to conserve the supply of PPE, and to identify new sources of PPE. We are interested in suggestions, recommendations, and potential actions from individuals who have relevant experience, especially from physicians, other health care professionals, and administrators in hospitals and other clinical settings. JAMA is inviting immediate suggestions, which can be added as online comments to this article.

@andrewwgithub thanks for the input! To be more specific, do you mean facility-wide policy to be one mask (N95?) per provider per day?

Yes one provider per day until the supply situation improves.

+data source:
https://gitter.im/covid19-healthsystemcapacity/community?at=5e77bc0e3a3acf64c1491943:

here's a continually updated #GetMePPEs spreadsheet for crowdsourced hospital facility needs and donation centers being set up: https://docs.google.com/spreadsheets/d/1txEanDkIrJ5GNfSk-zlXkTlB-bQPNRN_Y69qEwmdme8/htmlview?usp=sharing_eil&ts=5e77bb36&urp=gmail_link&sle=true
As a potential task after the 1st data release, someone could look at how we pull in this data automatically, join it to our facilities data, and visualize the crowdsourced needs and donation info per facility on our CCM map

What is the status of data sources for the project?

Thanks for your inputs @daveluo and @andrewwgithub.
TODOs
I am planning to spend some time on the modeling this week and may need to reach out for guidance from you all. I have been talking to a few Andrew's colleagues at NM.  The next key data points have all mentioned above are: PPE burn rates and usage as potential patients progress from testing to admission to discharge, a few admin and physitians from NM are willing to help to sort these key data points.

get all the data for hospital capacity on the map;
put convid-19 patients track data to the map;
read the peer-reviewed articles what the team has put: 1 - 2015 study and 2 - critical care surge capacity planning. (other things I missed?)
model and calculate hospital capacities and PPE needs around the Chicago area and:

reach out to NM to verify the outputs;
add missing data points, fix things;

make a plan for the next steps (run and predict other states).

If you want to combine the those two papers to create an estimate of what's needed, I think one way to do that is:

https://github.com/covidcaremap/covid19-healthsystemcapacity/blob/master/notebooks/processing/Generate_CCM_CareModel_Facility_Data.ipynb is a start to model Staff demands in the 3 scenarios mentioned in both of those papers: Conventional, Contingency, Crisis. It is based off of estimated ICU occupancy rates for each scenario, considering the number of ICU staffed beds in conventional and contingency scenarios, and All beds in the crisis scenario. It works backwards from there based on the table mentioned in the notebook to produce number of additional Physicians, Respiratory Therapists, and Critical Care Nurses required.
We are using beds as a proxy for patients, assuming that all the beds of that scenario are taken up by a patient. If there is data on a patient's time through the hospital for positive COVID19, then you could estimate the number of patients  based on bed counts
With an estimated number of patients over time and estimated staff needed, if there's data around how many PPEs are need per staff per patient, that should let you estimate the burn rate.

These models are all based on percentages and I'm not sure how accurate they are, so there may be much better approaches to this problem. However the CareModel work might help serve as a starting point.
Let me know if you have any problems running the notebooks, using the data, or any workflow things. Ideally this repository makes getting running on an analytics challenge like the one you're tackling a bit easier to spin up on.
If you end up brining in data to use and make a PR, here's a quick rundown of how I've been approaching keeping track of everything:

Download the data if it's not that big to be committed to the repo. If it is big, add downloading it to the processing/00_Download_Data.ipynb notebook. Add it to the data/external foldere.
Add source information to data/README.md
Data paths in notebooks should be built up with the data path helper methods; for example if you added external/data.csv, then you can get the full path (inside or outside docker) by:

from covidcaremap.data import external_data_path

the_path = external_data_path('data.csv')

The covidcaremap package has some constant values and some geospatial methods that are meant to be reused between notebooks. If you want to create reusable python functionality feel free to add it to that module.
Any data output by the notebook should go into the data/processed directory. You can get a path there by using the processed_data_path method similar to external_data_path. I've been committing the processed_data files even though they can be regenerated by notebooks; I think this allows people to run notebooks selectively and also puts up data that people may find useful (but should use at their own risk).
If a notebook genert data, it uses the published_data_path. This is data we'd advocate for people to use.
If you need to install any python libs, please put them in notebooks/requirements.txt. That way the docker/build script will install them into the container and other users won't have to think to hard about the install environment.

If there's any suggestions on how to improve this workflow please feel free to comment, chat, or make changes in a PR!

@williamratcliff

What is the status of data sources for the project?

I'm unsure what you are asking, can you clarify? Do you mean this project or the projects listed in the comments above yours? Thanks!

I need a few data layers today to put things together:

hospital capacity (staffed bed, ICU beds, # health workers)

covid patient prediction models;

PPE supplies

I'd love to get some guidance on:

which data layer for the hospital capacity I should use?
who is working on covid patient prediction models (#9)? Has someone started?

Andrew W look me into a few discussions with NM and they produce an IDOH report.

@Geoyi Adding what I posted in slack here in case it's helpful for you or others.
From https://usafacts.org/visualizations/coronavirus-covid-19-spread-map/ they have county level confirmed cases: https://static.usafacts.org/public/data/covid-19/covid_confirmed_usafacts.csv
Should we add a layer on counties' capacity (at least in terms of hospital beds) to handle the current and future estimated cases?
We will need to also merge in data on total population as the current layer is beds per 1000.

Looked over #68 - seems like a great start.
So if we take the county level confirmed cases mentioned above, could this be turned onto PPE estimates per county? That might be a first good step before the EPI model can project out caseload per county, if we get a model that can do that.
Where does the hospital/ICU capacity fit in? That could be supplied by the current dataset; updated ICU capacity numbers would be great but still looking for the best solution to collect that.

Thank you, @abarciauskas-bgse for sharing the data input.
Great points @lossyrob for using the confirmed cases at the county level to start. I talked to Jaline Gerardin at Northwestern U, she and her students have started EPI model for the Chicago area, and I think we may just wait a little to see what is coming out of it.
I'd say we wait until we have an updated and accurate hospital/ICU capacity, and make a plan from there. I am afraid everything is dynamically changing at the hospital side, for instance, they will make more beds/ICU beds if too many patients.
I am meeting with physicians again to debrief what data points we will need from them to track PPE needs at county level with the covid19 patient # changes.
Todo today : how can we estimate PPE needs when we have the confirmed cases at the county level

Qs to answer (☝ look at the diagram):

what is the ratio between the healthcare provider and patient at community testing to ED Presentation per day? what are the ratios look alike at In-Patient Care and Critical Care?

what is the percentage of other non-covid19 patients who will need PPEs that can't be neglected, and what the PPE needs looks like? Are these needs can be estimated before convid19 hospitals' average ICU occupation?

If we are looking at hourly/daily county-level covid19 confirmed cases, how should we envision the PPE needs through the whole patient flow? In the confirmed cases, we are only looking at after Home Care ☝, and what will be the best way to compute PPE needs through In Patient Care to Critical Care because a covid19 patient will stay for a week or more?

Last but not least, how do we define "PPE needs", large and well-funded hospitals have stocked up more PPEs but the physician in small hospitals are asked to reuse some of their PPE items?

I hope all the questions can be answered by physicians in the coming meeting.

Got some feedback from clinician friends that I've consolidated into a range for staffing ratios they're seeing:

Service
Staff
Staff:Patient ratio per shift

Emergency Dept
Physicians, Nurse Practitioners (NP), Physician Assistants (PA)
?

Nurses for general inpatient-level patients
1:6-9

Nurses for ICU-level patients
1:4

General Inpatient
Physicians, Nurse Practitioners (NP), Physician Assistants (PA)
1:5-6

Nurses and Patient Care Associates (PCA)
1:2-4

ICU
Physicians, NPs
1:3-4

Respiratory Therapists
1:4-6

Nurses and PCAs
1:1-2

PPE needs estimation has been done and updated  here (cc: @andrewwgithub)
Note: current PPE needs were estimated from confirmed covid19 cases (dataset from USAFacts) and follow the above diagram ☝ which can be underestimated for a few aspects;

The estimation was updated until 25th, and most cases grew on 26th in the state, which was not captured yet but we will continue to work on improving;

We're working with physicians and current PPEs needs through the patient flow (diagram above) still underestimated the needs, we are working on improving this and will reflect the improvement through the current codebase.

Still waiting on confirmation from some ED colleagues, here some data from a 2014 survey for patients per hour:
https://www.acepnow.com/article/emergency-department-benchmarking-alliance-releases-2014-data-on-staffing-physician-productivity/2/

My ED colleagues have confirmed the patient per hour ratio looks good, some EDs have APPs practice independently, while at large academic centers, the APP must have an attending physician.

the online report for PPE need/demand is up here: https://rpubs.com/Geoyi/PPE_need_usa_convid19.
The demand has been updated until April 3rd.

This is great thank you!


Document return solve status

We have result.status in the output of the optimizer but it is an un-interpretable integer now. Can we return a string that maps this integer to the output code explained here?

Ah, it's printed on the screen when IpOpt exits:
So result.status = -1 means EXIT: Maximum Number of Iterations Exceeded.


Killed player remains running on the spot he is killed

Hey dev, I am using compquality preset and I find a problem using it since the update last week(should be 6.5.4.
Reproducing the issue

Kill a player
The player sometimes remains in the air/ one the ground where he is killed, note that ragdoll is disabled by default in this preset.

What makes it special is, sometimes the player model is like running when this happen, usually it stays for a few minutes.

The lastest one
On Thu, Apr 12, 2018, 8:09 AM mastercoms<EMAIL_ADDRESS>wrote:

What mastercomfig version are you on now?
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/mastercoms/mastercomfig/issues/42#issuecomment-380679751,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AVOhpYu-k3Ad7QCoRIxLdGrWxnu-yuOHks5tnuF6gaJpZM4TRJeI
.

Could you say the version number? It's easier for tracking purposes.

ok, let me find it, and i am making a clip of this


Tutorial on iCub blink or face emotion?

Now, we want to control the physical robot as naturally as possible. Do you have any tutorials on iCub blink and face emotion? Or any website I  can refer to?

Hi @YANGGANG1971
is your robot iCubOsaka01? Just asking beforehand as there exist different hardware versions underneath the use of facial expressions.

yes, iCubOsaka01

Here we go:

Application scripts for running the face expressions are contained in the main bundle and described in our official documentation. While reading the doc, bear in mind that your robot iCubOsaka01 doesn't mount the latest RFE board.
We have also developed in the past a specific module controlling the blinking behavior. It's called iCubBlinker and is hosted at https://github.com/robotology/funny-things.

Thanks, I also tried that.

Well, the bash scripts (e.g. cycle.sh) do simply rely on standard port-based communication.
Thereby, the following command:
echo "set all hap"  | yarp rpc /icub/face/emotions/in

has clearly its C++ counterpart:
yarp::os::BufferedPort<yarp::os::Bottle> port;
port.open("/emotions/out");
yarp::os::Network::connect(port.getName(), "/icub/face/emotions/in", "tcp");

yarp:os::Bottle& cmd = port.prepare();
cmd.clear();
cmd.addString("set all hap");
port.writeStrict();

Thanks, I will test it on our physical robot.

Following your suggestion, it seems something wrong while opening icubDemoScripts to call modules followed by the tutorial
https://github.com/robotology/funny-things#step-by-step-guide. Before that,  I have execute yarpserver --write and iCub_SIM.

Also, for the physical robot, the error even occurred at compiling and see the figure blow.

I guess it is due to yarp version or iCub_main version. In fact, I do not update the version, because I have many program relying on the current software version.
My desktop configuration is as follows,
yarp 3.1.0
Ubuntu 16.04
cmake 3.5.1
swig 3.0.8
icub_main v1.12.0
Do you have any idea for addressing this issue, or other way to realize iCub blink?

Hi @YANGGANG1971
From the error reported by yarpmanager

it seems a problem related to the availability of yarprun. So, you'd better check this out.
Unfortunately, funny-things is NOT versioned according to our Software Versioning Table, so you ought to go back in the funny-things Git history up to the point in time when your yarp and icub-main versions have been delivered. This is a bit tricky but it's the fastest workaround, I believe.

@YANGGANG1971 , the iCubOsaka01 was last updated around September 2018 if I remember correctly. Hopefully that information will narrow down the git history search.

Hi @YANGGANG1971
From the error reported by yarpmanager

it seems a problem related to the availability of yarprun. So, you'd better check this out.
Unfortunately, funny-things is NOT versioned according to our Software Versioning Table, so you ought to go back in the funny-things Git history up to the point in time when your yarp and icub-main versions have been delivered. This is a bit tricky but it's the fastest workaround, I believe.

I am a little bit confused. yarprun aims at killing, monitoring other programs on diff. set of machines. If I only use the iCub sim., do I need to yarprun --server /SERVERPORT? If indeed, how do I set the server name? p104?

The yarprun is a broker that is required to be running on machine B when you aim to launch an application from the machine A (that is where you rely on yarpmanager, for example) on machine B. Therefore, yarprun doesn't serve only for killing/monitoring remote applications but primarily for starting/stopping them.
When you want to launch an application on the same machine of yarpmanager you may replace yarprun broker with localhost.
From the snapshot you posted above, only simFaceExpressions seems correctly configured to run locally using the broker localhost, whereas the remainder of the modules are needing the terminal port of the yarprun server.
So, either you launch yarprun --server /terminal on your shell prior to clicking on Run All button, or simpler edit the application file to replace terminal with localhost.

Thanks, it works. But the new problem occurred as follows.

Error: cannot run gaze.lua on locahost: Can't execute command because no such file or directory.
I am wondering by default, where gaze.lua should be put or where the .xml will first look for? Or how do I address this issue above?

The correct name of the module is indeed simFaceExpressions: fixed in https://github.com/robotology/funny-things/commit/927ffa0efbb1f81485e176a16e87a5a67bdd573a. You ought to edit the application file then.
The LUA script gaze.lua should get installed correctly in a location that shall be in the path. So, check it out by doing:
$ which gaze.lua

However, LUA does also need to be installed correctly. Follow these instructions: http://www.yarp.it/yarp_swig.html#yarp_swig_lua.
For testing blinking, gaze.lua is not necessary though.

In iCub Sim, now blinking is working. Thanks, but the gaze.lua still cannot work, although I have installed lua. It seems no affecting the blinking function.

The physical iCub is also working. Thanks.

The physical iCub is also working. Thanks.

Good to know 👍🏻
The script gaze.lua has nothing to do with blinking as it merely asks the module iKinGazeCtrl to move the gaze toward some given locations, even randomly.
Did you try out one of the LUA examples available in YARP?
This one is a good start: https://github.com/robotology/yarp/blob/master/bindings/lua/examples/example.lua.

Thx,  I will try.


Fix broken links in README.md

The contributing url and code of conduct hyperlink needed to be replaced with the github repo owner's username.

Coverage remained the same at 91.429% when pulling 20b1845534cdbd2e8d6a2ce636bab3fd2ef1ea23 on alexanderdavidpan:patch-1 into 0e7e8569dcbacd86f81a1678fa8ac0a14a3e453a on ankitsamarthya:master.


improve and optimize checkpointing/state system

The checkpointing system is pretty rudimentary at the moment, it simply checks if a function in workflow.task_list has been run (by name) and skips the function if it has. This is not optimal as it means a lot of repeated simulations will take place. E.g., if 60 forward simulations are launched and all but one finishes, restarting the workflow will cause all 60 jobs to be re-run.
Additionally, there is no clear way to interact with the state file. At the moment it involves directly editing the text file, but that is a bit clunky and prone to error. It would be great to have a command line tool to check, edit and clear a statefile. The state file should likely also be a hidden file to prevent casual users from accidentally deleting or editing it manually.
Tasks to be completed are then:

[ ] Improve task-level checkpointing (this can be done by checking solver logs, or generated waveforms in the 'traces' dir.)
[ ] Create a seisflows state command line option to manipulate the state file
[ ] Generate the entire state file at the setup stage, as opposed to building it throughout the first iteration
[ ] Make the state file a hidden file

Hi @bch0w, I implemented a task-level checkpoint system. I have been testing it during the past months and it seems to work pretty well. I will submit a pull request this week!

Hi @evcano, just wondering if you have any updates on your task-level checkpoint system? No pressure, just something I have been thinking about recently and I would be excited to see your work on it!

Hi @bch0w, sorry for being absent!
I've been busy writing a manuscript but I will soon push my changes. You can see some here:
https://github.com/evcano/seisflows/pull/4
But I will push them soon

Hi @bch0w,
I am cleaning up the changes I made. I will submit the pull request tomorrow!

Working on this in https://github.com/adjtomo/seisflows/tree/feature-checkpoint


%% issue with sprite path

When we using %%, it means we want to @extend code from sprite.scss to our main.scss
And here is the problem
we can have such structure
-scss
---main.scss
---css
------sprite.scss
------svg
---------sprite.css-hdbch.svg
and in sprite.scss code we would have something like
%svg-common {
background-repeat: no-repeat;
background-image: url(svg/sprite.css-3feb3afe.svg);
}
%svg-ico-btn-slide-up {
background-position: 72.28260869565217% 0;
@extend %svg-common;
}
but when we exporting this code to main.scss
path to sprite wouldn't change
instead /css/svg/sprite.css-hdbch.svg sprite in /scss/css/svg/sprite.css-hdbch.svg folder
so it would be better to make sprite with its folders exactly in the root folder

Hey @berserkmustdie,
I'm sorry but it's definitely beyond the scope of svg-sprite to "know" and fulfil the specifics of your Sass setup. I can think of two possible solutions, but both are up to you:

As you already suggested: rearrange your Sass resources or
derive a custom Mustache template and add that missing directory by hardcoding it or — maybe better — injecting it via a custom template variable.

Cheers,
Joschi


Change screen co-ordinate system to top-left corner

Getting shifted from other mobile frameworks like: React Native and Flutter, it's difficult for me to work with bottom-left co-ordinate system. Is there any way to alter this, or anything I can do to change it for all my projects (by changing on kivy source or anything...).
If it could be possible then, place it as a feature in Kivy if possible.

I think it's unrealistic to make that a configuration option in kivy, or even to update the source manualy to make that change, it's a lot of work for no gain. What problems are you getting from this coordinate system? It shouldn't take a long time to get used to the idea that positive is up, negative is down.

I think it's unrealistic to make that a configuration option in kivy, or even to update the source manualy to make that change, it's a lot of work for no gain. What problems are you getting from this coordinate system? It shouldn't take a long time to get used to the idea that positive is up, negative is down.

Suppose I want to display lets say label on the very top of the screen so for that maybe I need to make the position_hint property set to 0.0 on the x direction and 1.0 to the y direction, But the label co-ordinates are also counted from bottom left so in this case the label will not be shown on the screen because its sitting on top of the upper bound of the screen.

Suppose I want to display lets say label on the very top of the screen so for that maybe I need to make the position_hint property set to 0.0 on the x direction and 1.0 to the y direction, But the label co-ordinates are also counted from bottom left so in this case the label will not be shown on the screen because its sitting on top of the upper bound of the screen.

As @tshirtman said it's unrealistic for kivy to change like this now.
Kivy Widgets do have properties to help with this, such as top or right. Please read the Widget documentation.

As @tshirtman said it's unrealistic for kivy to change like this now.
Kivy Widgets do have properties to help with this, such as top or right. Please read the Widget documentation.


Add gromacs openmp config

Added the gromacs openmp config
The config works with for the systems: cts1 and x86 (on cts1)
The config fails for ats4 due to a cmake error within gromacs (this probably needs to be fixed within the gromacs build system)
Running gromacs with hwloc on results in a runtime error on cts1 (hence the code is built with hwloc off)

The config fails for ats4 due to a cmake error within gromacs (this probably needs to be fixed within the gromacs build system)

Let's try to repro this with the latest GROMACS release, see my comment on #61.

Running gromacs with hwloc on results in a runtime error on cts1 (hence the code is built with hwloc off)

Same as above; also note that if CTS1 is this, hwloc is not critical since we have the internal hardware topology detection as a fallback.


PWM support

Great stuff. I found out about your project following a link from the original at the tutsplus.com. One thing I would find useful: PWM. Controlling the speed of the motors. Check out this link

@icapan Thanks for the interest! I was considering adding support for PWM ages ago but never got around to it - there are a few things (e.g. #20) that I would like to add but I'm currently spending most of my time on other projects.
If you've got an L293D chip set up at the moment and feel like contributing, feel free to have a go at adding PWM support yourself! I'd be happy to get back into this project and help

I have a chip and several DC motors, I intend to try to configure it (I wasn't successful so far), but my programming knowledge is limited, so don't expect too much from me :)

@icapan That's alright, just feel free to fork this repo and submit a pull request if you get anywhere

Some useful links:

http://raspi.tv/2013/rpi-gpio-0-5-2a-now-has-software-pwm-how-to-use-it
https://pypi.python.org/pypi/RPi.GPIO
https://www.coursera.org/learn/raspberry-pi-platform/lecture/DMArS/lecture-2-2-pulse-width-modulation

Just a mention for @mortenmoulder as they were interested in PWM support from #61.


Add macos aarch64

Github enabled ARM M1 chips in Actions.
https://github.blog/2023-10-02-introducing-the-new-apple-silicon-powered-m1-macos-larger-runner-for-github-actions/

I'm closing this PR in favor of #58


How to get newest docker engine on MacOS with open-source Docker?

After installing both docker and docker-machine from Homebrew and setting up docker-machine with a VirtualBox driver, the docker version command reports that my client version is 20.10.8 whereas the server version is 19.03.12.
I would like to upgrade to the latest server version (20.10.8 at the time of posting), that should still be open source if I'm not mistaken.
As it appears that docker-machine is not actively maintained anymore, what is the correct way to upgrade to the latest docker server version (without using proprietary Docker software)? Are there any instructions anywhere? Thanks

Version 19.03 is indeed the last and final version of boot2docker, the Linux distribution of Docker machine. It is now deprecated
You can use an Ubuntu VM and install and upgrade Docker in it, but docker-machine only does this for cloud drivers (and doesn't support 20.10)

Like so: https://boot2podman.github.io/2020/07/22/machine-replacement.html
You could use another virtualization solution, by doing the same commands as in the Vagrantfile... The docker client (installed with brew) uses ssh to talk to the socket in the VM.

examples docker-machine with vagrant.
<example>/
|__ Vagrantfile
|__ docker-compose.yml ... example for docker container(php:apache).
|__ index.php          ... example php source.

Vagrantfile
Vagrant.configure("2") do |config|
  config.vm.box = "ubuntu/hirsute64"

  config.vm.network "private_network", ip: "<IP_ADDRESS>"

  # mapping working dir to to vm as same path.
  config.vm.synced_folder ENV["PWD"], ENV["PWD"]
end

docker-compose.yml
version: "3"
services:
  php:
    image: "php:apache"
    volumes:
      - ".:/var/www/html"
    ports:
      - "80:80"

index.php
<?php
phpinfo();

launch vagrant vm.
vagrant up

register to docker-machine with generic driver.
docker-machine create --driver generic \
	       --generic-ip-address "<IP_ADDRESS>" \
	       --generic-ssh-key $PWD/.vagrant/machines/default/virtualbox/private_key \
	       --generic-ssh-port 22 \
	       --generic-ssh-user "vagrant" \
	       docker-with-vagrant-vm

apply docker-machine environment variables.
eval $(docker-machine env docker-with-vagrant-vm)

list docker machines.
docker-machine ls
NAME                     ACTIVE   DRIVER       STATE     URL                         SWARM   DOCKER      ERRORS
docker-with-vagrant-vm   *        generic      Running   tcp://<IP_ADDRESS>:2376            v20.10.8

launch docker container.
docker-compose up -d

open browser.
open http://<IP_ADDRESS>/

Nice! I was thinking to do a vagrant-machine to wrap it (Vagrant), but in retrospect a Vagrant machine-driver would have been better...

Here is another project that I'm following closely: https://github.com/abiosoft/colima
It is the most promising Docker Desktop replacement for Mac I have seen and I have had the most success with it so far.


Add assets and build config to gitignore

Assets and build config files and folders for both iOS and Android need to be added to gitignore. We need to ensure the build config files and folders done end up getting added to commit and replacing the build configuration.

This is done.


xSQLServerLogin: Removed ShouldProcess; Added Parameters

Updated the xSQLServerLogin resource to remove all references to the ShouldProcess cmdlet. Added the following parameters for the SqlLogin type:

LoginMustChangePassword
LoginPasswordExpirationEnabled
LoginPasswordPolicyEnforced

Updated the examples and unit tests.
This Pull Request (PR) fixes the following issues:
Remove ShouldProcess: Fixes #240
Add SqlLogin Password Requirements: Fixes #251

[X] Change details added to Unreleased section of CHANGELOG.md?
[X] Added/updated documentation, comment-based help and descriptions in .schema.mof files where appropriate?
[X] Examples appropriately updated?
[X] New/changed code adheres to Style Guidelines?
[X] Unit and (optional) Integration tests created/updated where possible?

This change is

Hi @randomnote1, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Dan Reist). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

@randomnote1 Great work! These changes are a great addition!
But would you mind creating and issue for the additional parameters so others have chance to discuss it? And also add that issue to the PR description  so it automatically closes when this PR is merged.

Absolutely. Issue #251 created for tracking and discussion.

Impressive work @randomnote1! I continue to review the tests when these comments are resolved.

Reviewed 6 of 9 files at r1, 1 of 2 files at r2.
Review status: 7 of 9 files reviewed at latest revision, 27 unresolved discussions.

README.md, line 205 at r2 (raw file):

* **Ensure**: The specified login is Present or Absent.
* **Name**: (Key) The name of the SQL login.

Could you align this description with the schema?

README.md, line 206 at r2 (raw file):

* **Ensure**: The specified login is Present or Absent.
* **Name**: (Key) The name of the SQL login.
* **LoginType**: The type of login to be created. If LoginType is 'WindowsUser' or 'WindowsGroup' then provide the name in the format DOMAIN\name.

The resources throws if one uses a LoginType in the ValidateSet that is not supported. I don't see a problem leaving that in, since this is still a experimental resource, but could you add to this description which login types are not supported yet?
Could you also please add saying what the default value is?

README.md, line 210 at r2 (raw file):

* **SQLInstanceName**: (Key) Name of the SQL instance to be configured.
* **LoginCredential**: If LoginType is 'SqlLogin' then a PSCredential is needed for the password to the login.
* **LoginMustChangePassword**: Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.

Could you also please add saying what the default value is?

README.md, line 211 at r2 (raw file):

* **LoginCredential**: If LoginType is 'SqlLogin' then a PSCredential is needed for the password to the login.
* **LoginMustChangePassword**: Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.
* **LoginPasswordExpirationEnabled**: Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

README.md, line 212 at r2 (raw file):

* **LoginMustChangePassword**: Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.
* **LoginPasswordExpirationEnabled**: Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.
* **LoginPasswordPolicyEnforced**: Specifies if the login password is required to conform to the password policy specified in the system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 13 at r2 (raw file):

    .PARAMETER SQLServer
    Hostname of the SQL Server to retrieve the login from.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 24 at r2 (raw file):

Mandatory=$true

Could you change this to `Mandatory = $true' (with spaces around the equal sign). Do this for all other occurrences too.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 30 at r2 (raw file):

        [Parameter(Mandatory=$true)]
        [System.String]
        $SQLServer = $env:COMPUTERNAME,

Please remove default value here since it is mandatory.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 37 at r2 (raw file):

    )

    Import-SQLPSModule

You don't need Import-SQLPSModule here since Connect-SQL load's the assemblies? We only need to load SQLPS when cmdlets are used?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 98 at r2 (raw file):

    .PARAMETER LoginMustChangePassword
    Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 101 at r2 (raw file):

    .PARAMETER LoginPasswordExpirationEnabled
    Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 104 at r2 (raw file):

    .PARAMETER LoginPasswordPolicyEnforced
    Specifies if the login password is required to conform to the password policy specified in the system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 158 at r2 (raw file):

    )

    Import-SQLPSModule

Same here, this should not be needed?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 187 at r2 (raw file):

                    {
                        New-VerboseMessage -Message "Setting PasswordPolicyEnforced to '$LoginPasswordPolicyEnforced' for the login '$Name' on the '$SQLServer\$SQLInstanceName' instance."
                        $login.PasswordPolicyEnforced = $LoginPasswordPolicyEnforced

If the account already exist. Shouldn't is reset the password if you enforce the policy? Otherwise the password might be worng. Think that the user wanted to change the password at the same time as enforcing the policy?
If is should never set a password when the account already exist, then it should not force to have the $LoginCredential set above?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 212 at r2 (raw file):

                switch ($LoginType)
                {
                    SqlLogin

Minor. This seems to work...Did not know this. But I read this as a variable at first. Wouldn't be more natural to have this inside single quotes 'SqlLogin' so that it is easy read that it is the string we are comparing to?
But, there is no guideline saying one cannot do this so you can also keep it if you want. :)

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 294 at r2 (raw file):

    .PARAMETER LoginMustChangePassword
    Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 297 at r2 (raw file):

    .PARAMETER LoginPasswordExpirationEnabled
    Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 300 at r2 (raw file):

    .PARAMETER LoginPasswordPolicyEnforced
    Specifies if the login password is required to conform to the password policy specified in the system security policy. Only applies to SQL Logins.

Could you also please add saying what the default value is?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 362 at r2 (raw file):

        SQLInstanceName = $SQLInstanceName
    }
    $loginInfo = Get-TargetResource @getParams

Could you add a blank line before this one?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 4 at r2 (raw file):

class MSFT_xSQLServerLogin : OMI_BaseResource
{
    [Write, Description("The specified login is Present or Absent."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;

Could you also please add saying what the default value is? Aligning with the readme.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 5 at r2 (raw file):

{
    [Write, Description("The specified login is Present or Absent."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
    [Key, Description("The name of the login.")] String Name;

Align this description with the README.md

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 6 at r2 (raw file):

    [Write, Description("The specified login is Present or Absent."), ValueMap{"Present","Absent"}, Values{"Present","Absent"}] String Ensure;
    [Key, Description("The name of the login.")] String Name;
    [Write, Description("The type of login to be created. If LoginType is 'WindowsUser' or 'WindowsGroup' then provide the name in the format DOMAIN\name."), ValueMap{"WindowsUser","WindowsGroup","SqlLogin","Certificate","AsymmetricKey","ExternalUser","ExternalGroup"}, Values{"WindowsUser","WindowsGroup","SqlLogin","Certificate","AsymmetricKey","ExternalUser","ExternalGroup"}] String LoginType;

Could you also please add saying what the default value is? Aligning with the readme.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 10 at r2 (raw file):

    [Key, Description("Name of the SQL instance to be configured.")] String SQLInstanceName;
    [Write, EmbeddedInstance("MSFT_Credential"), Description("If LoginType is 'SqlLogin' then a PSCredential is needed for the password to the login.")] String LoginCredential;
    [Write, Description("Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.")] Boolean LoginMustChangePassword;

Could you also please add saying what the default value is? Aligning with the readme.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 11 at r2 (raw file):

    [Write, EmbeddedInstance("MSFT_Credential"), Description("If LoginType is 'SqlLogin' then a PSCredential is needed for the password to the login.")] String LoginCredential;
    [Write, Description("Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.")] Boolean LoginMustChangePassword;
    [Write, Description("Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.")] Boolean LoginPasswordExpirationEnabled;

Could you also please add saying what the default value is? Aligning with the readme.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 12 at r2 (raw file):

    [Write, Description("Specifies if the login is required to have its password change on the next login. Only applies to SQL Logins.")] Boolean LoginMustChangePassword;
    [Write, Description("Specifies if the login password is required to expire in accordance to the operating system security policy. Only applies to SQL Logins.")] Boolean LoginPasswordExpirationEnabled;
    [Write, Description("Specifies if the login password is required to conform to the password policy specified in the system security policy. Only applies to SQL Logins.")] Boolean LoginPasswordPolicyEnforced;

Could you also please add saying what the default value is? Aligning with the readme.

en-US/xSQLServer.strings.psd1, line 46 at r2 (raw file):

PasswordValidationFailed = Creation of the login '{0}' failed due to the following error: {1}
LoginCreationFailed = Creation of the login '{0}' failed.
LoginTypeNotImplemented = The login type '{0}' is not implemented in this module.

maybe 'resource' instead of 'module'?

en-US/xSQLServer.strings.psd1, line 47 at r2 (raw file):

it must be set to 'Mixed'.

Change to '...it must be set to 'Mixed' authentication mode.'?

Comments from Reviewable

Reviewed 1 of 9 files at r1, 1 of 2 files at r2.
Review status: all files reviewed at latest revision, 27 unresolved discussions.

Comments from Reviewable

Thanks!

Review status: 3 of 9 files reviewed at latest revision, 28 unresolved discussions.

README.md, line 205 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you align this description with the schema?

Done.

README.md, line 206 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
The resources throws if one uses a LoginType in the ValidateSet that is not supported. I don't see a problem leaving that in, since this is still a experimental resource, but could you add to this description which login types are not supported yet?
Could you also please add saying what the default value is?

Done.
My original intention was to type the parameter, but that didn't work since SQLPS doesn't get loaded until the functions run.

README.md, line 210 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

README.md, line 211 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

README.md, line 212 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 13 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Removed the default value since they are mandatory parameters.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 24 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

Mandatory=$true

Could you change this to `Mandatory = $true' (with spaces around the equal sign). Do this for all other occurrences too.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 30 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please remove default value here since it is mandatory.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 37 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You don't need Import-SQLPSModule here since Connect-SQL load's the assemblies? We only need to load SQLPS when cmdlets are used?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 98 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 101 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 104 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 158 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same here, this should not be needed?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 187 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
If the account already exist. Shouldn't is reset the password if you enforce the policy? Otherwise the password might be worng. Think that the user wanted to change the password at the same time as enforcing the policy?
If is should never set a password when the account already exist, then it should not force to have the $LoginCredential set above?

The default behavior in SQL 2016 doesn't automatically require the password to be reset when selecting PasswordPolicyEnforced.
That being said, I forgot to account for changing the password via DSC. I updated the code so that the password should be set on every run if a password is specified.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 212 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Minor. This seems to work...Did not know this. But I read this as a variable at first. Wouldn't be more natural to have this inside single quotes 'SqlLogin' so that it is easy read that it is the string we are comparing to?
But, there is no guideline saying one cannot do this so you can also keep it if you want. :)

I think I went the opposite way, since default doesn't need quotes I made SqlLogin not have quotes as well.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 294 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 297 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 300 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 362 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you add a blank line before this one?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 4 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is? Aligning with the readme.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 5 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Align this description with the README.md

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 6 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is? Aligning with the readme.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 10 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is? Aligning with the readme.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 11 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is? Aligning with the readme.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.schema.mof, line 12 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Could you also please add saying what the default value is? Aligning with the readme.

Done.

en-US/xSQLServer.strings.psd1, line 46 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
maybe 'resource' instead of 'module'?

Done.

en-US/xSQLServer.strings.psd1, line 47 at r2 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

it must be set to 'Mixed'.

Change to '...it must be set to 'Mixed' authentication mode.'?

Done.

Comments from Reviewable

Reviewed 6 of 6 files at r3.
Review status: all files reviewed at latest revision, 30 unresolved discussions.

 Should be indented one more.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 40 at r3 (raw file):

    $mockSqlLoginBadPassword = 'pw' | ConvertTo-SecureString -AsPlainText -Force
    $mockSqlLoginCredentialBadpassword = New-Object System.Management.Automation.PSCredential( $mockSqlLoginUser, $mockSqlLoginBadPassword )

Change to 'Password' with upper 'P'

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 43 at r3 (raw file):

    $mockSqlLoginReusedPassword = 'reused' | ConvertTo-SecureString -AsPlainText -Force
    $mockSqlLoginCredentialReusedpassword = New-Object System.Management.Automation.PSCredential( $mockSqlLoginUser, $mockSqlLoginReusedPassword )

Change to 'Password' with upper 'P'

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 46 at r3 (raw file):

    $mockSqlLoginOtherPassword = 'other' | ConvertTo-SecureString -AsPlainText -Force
    $mockSqlLoginCredentialOtherpassword = New-Object System.Management.Automation.PSCredential( $mockSqlLoginUser, $mockSqlLoginOtherPassword )

Change to 'Password' with upper 'P'

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 177 at r3 (raw file):

                $result.Ensure | Should Be 'Present'
                $result.LoginType | Should Be 'SqlLogin'
                $result.LoginMustChangePassword | Should Not Be $null

Chaneg this and the two below to Should Not BeNullOrEmpty?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 188 at r3 (raw file):

                $result.Ensure | Should Be 'Present'
                $result.LoginType | Should Be 'WindowsUser'

You should "reverse" the check from the previous. So these are not set? Or even better, test that they are not included in the hash table, because that are the intended behavior?
$result.LoginMustChangePassword | Should BeNullOrEmpty
$result.LoginPasswordExpirationEnabled | Should BeNullOrEmpty
$result.LoginPasswordPolicyEnforced | Should BeNullOrEmpty

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 197 at r3 (raw file):

                $result.Ensure | Should Be 'Present'
                $result.LoginType | Should Be 'WindowsGroup'

Same as previous comment?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 210 at r3 (raw file):

            It 'Should return $true when the specified Windows user is Absent' {
                $testTargetResource_WindowsUserAbsent_EnsureAbsent = $testTargetResource_WindowsUserAbsent.Clone()
                $testTargetResource_WindowsUserAbsent_EnsureAbsent.Add( 'Ensure','Absent' )

Suggestion and Minor. Instead of adding parameter Ensure for each It-block. Couldn't you add that to a variable in the Context-block? Then add the parameters that differ (clone) in the It-block?
Not really thought this thru, but my intention is if we can remove code that is repeated but still not make more rows, but rather less code. If my suggestion will end up with more rows of code, then leave as-is.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 266 at r3 (raw file):

            It 'Should return $false when the specified Windows user is Absent' {
                $testTargetResource_WindowsUserAbsent_EnsurePresent = $testTargetResource_WindowsUserAbsent.Clone()
                $testTargetResource_WindowsUserAbsent_EnsurePresent.Add( 'Ensure','Present' )

Repeating code in each it-block. Same comment as above.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 313 at r3 (raw file):

                $testTargetResource_SqlLoginPresentWithDefaultValues_EnsurePresent.Add( 'Ensure','Present' )

                ( Test-TargetResource @testTargetResource_SqlLoginPresentWithDefaultValues_EnsurePresent ) | Should Be $true

Minor. I see a "red dot" here at the end of the line? Is it a blank space, a tab, or neither?
Just curious, I have seen it now and then. If it is a space it is minor, if it is a tab, then I have to start comment on these as well :smile:

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 392 at r3 (raw file):

                $setTargetResource_WindowsUserPresent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_WindowsUserPresent_EnsureAbsent

You need to test (assert) that Drop() is actually called.
That is why Drop() was wrapped in a helper function before. See Remove-SqlLogin in original code and tests. Then we could mock and assert that function.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 404 at r3 (raw file):

                $setTargetResource_WindowsGroupPresent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_WindowsGroupPresent_EnsureAbsent

You need to test (assert) that Drop() is actually called.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 416 at r3 (raw file):

                $setTargetResource_SqlLoginPresent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_SqlLoginPresent_EnsureAbsent

You need to test (assert) that  Drop() is actually called.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 428 at r3 (raw file):

                $setTargetResource_WindwsUserAbsent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_WindwsUserAbsent_EnsureAbsent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 440 at r3 (raw file):

                $setTargetResource_WindwsGroupAbsent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_WindwsGroupAbsent_EnsureAbsent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 452 at r3 (raw file):

                $setTargetResource_SqlLoginAbsent_EnsureAbsent.Add( 'Ensure','Absent' )

                Set-TargetResource @setTargetResource_SqlLoginAbsent_EnsureAbsent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 466 at r3 (raw file):

                $setTargetResource_WindowsUserAbsent_EnsurePresent.Add( 'Ensure','Present' )

                Set-TargetResource @setTargetResource_WindowsUserAbsent_EnsurePresent

You need to test (assert) that there is no call to Drop() and one call to Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 478 at r3 (raw file):

                $setTargetResource_WindowsGroupAbsent_EnsurePresent.Add( 'Ensure','Present' )

                Set-TargetResource @setTargetResource_WindowsGroupAbsent_EnsurePresent

You need to test (assert) that there is no call to Drop() and one call to Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 491 at r3 (raw file):

                $setTargetResource_SqlLoginAbsent_EnsurePresent.Add( 'LoginCredential',$mockSqlLoginCredential )

                Set-TargetResource @setTargetResource_SqlLoginAbsent_EnsurePresent

You need to test (assert) that there is no call to Drop() and one call to Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 505 at r3 (raw file):

                $setTargetResource_SqlLoginAbsent_EnsurePresent.Add( 'LoginMustChangePassword',$false )

                Set-TargetResource @setTargetResource_SqlLoginAbsent_EnsurePresent

You need to test (assert) that there is no call to Drop() and one call to Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 541 at r3 (raw file):

                $setTargetResource_WindowsUserPresent_EnsurePresent.Add( 'Ensure','Present' )

                Set-TargetResource @setTargetResource_WindowsUserPresent_EnsurePresent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 553 at r3 (raw file):

                $setTargetResource_WindowsGroupPresent_EnsurePresent.Add( 'Ensure','Present' )

                Set-TargetResource @setTargetResource_WindowsGroupPresent_EnsurePresent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 566 at r3 (raw file):

                $setTargetResource_SqlLoginPresent_EnsurePresent.Add( 'LoginCredential',$mockSqlLoginCredential )

                Set-TargetResource @setTargetResource_SqlLoginPresent_EnsurePresent

You need to test (assert) that there are no calls to Drop() or Create() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 580 at r3 (raw file):

                $setTargetResource_SqlLoginPresent_EnsurePresent_LoginPasswordExpirationEnabled.Add( 'LoginPasswordExpirationEnabled',$false )

                Set-TargetResource @setTargetResource_SqlLoginPresent_EnsurePresent_LoginPasswordExpirationEnabled

You need to test (assert) that there is a call to Alter() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 594 at r3 (raw file):

                $setTargetResource_SqlLoginPresent_EnsurePresent_LoginPasswordPolicyEnforced.Add( 'LoginPasswordPolicyEnforced',$false )

                Set-TargetResource @setTargetResource_SqlLoginPresent_EnsurePresent_LoginPasswordPolicyEnforced

You need to test (assert) that there is a call to Alter() here.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 600 at r3 (raw file):

            }

            It 'Should throw when creating a SQL Login if the LoginMode is not Mixed' {

Change all It-blocks that "throws" so that their description include 'Should throw the correct error...'

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 656 at r3 (raw file):

            }

            It 'Should throw PasswordValidationFailed when updating a SQL Login' {

Maybe use the same description as the one before instead of  'PasswordValidationFailed'.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 669 at r3 (raw file):

            }

            It 'Should throw PasswordChangeFailed when updating a SQL Login' {

Se previous comment. Maybe write out 'PasswordChangeFailed'?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 682 at r3 (raw file):

            }

            It 'Should throw PasswordChangeFailed when updating a SQL Login' {

This It-block is a duplicate to the one before?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 695 at r3 (raw file):

Should throw when creating a SQL Login fails

Maybe 'Should throw the correct error when SQL login cannot be created'?

Comments from Reviewable

Review status: 3 of 9 files reviewed at latest revision, 31 unresolved discussions.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 204 at r3 (raw file):

hrow New-TerminatingError -ErrorType PasswordChangeFailed -FormatArgs $Name -ErrorCategory NotSpec

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 205 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This brace seems off. Should be indented one more.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 40 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change to 'Password' with upper 'P'

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 43 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change to 'Password' with upper 'P'

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 46 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change to 'Password' with upper 'P'

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 177 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Chaneg this and the two below to Should Not BeNullOrEmpty?

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 188 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You should "reverse" the check from the previous. So these are not set? Or even better, test that they are not included in the hash table, because that are the intended behavior?
$result.LoginMustChangePassword | Should BeNullOrEmpty
$result.LoginPasswordExpirationEnabled | Should BeNullOrEmpty
$result.LoginPasswordPolicyEnforced | Should BeNullOrEmpty

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 197 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous comment?

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 210 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Suggestion and Minor. Instead of adding parameter Ensure for each It-block. Couldn't you add that to a variable in the Context-block? Then add the parameters that differ (clone) in the It-block?
Not really thought this thru, but my intention is if we can remove code that is repeated but still not make more rows, but rather less code. If my suggestion will end up with more rows of code, then leave as-is.

Thought about it, but I think I'll sacrifice some readability in order to reduce a few lines of code. I see where you're coming from though, and if I can think of a good way to organize it I'll make the change. Leaving this go for the time being.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 266 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Repeating code in each it-block. Same comment as above.

See above comment

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 313 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Minor. I see a "red dot" here at the end of the line? Is it a blank space, a tab, or neither?
Just curious, I have seen it now and then. If it is a space it is minor, if it is a tab, then I have to start comment on these as well :smile:

I think it's a space. I deleted it, so we can see on the next revision if the "red dot" is gone.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 392 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that Drop() is actually called.
That is why Drop() was wrapped in a helper function before. See Remove-SqlLogin in original code and tests. Then we could mock and assert that function.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 404 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that Drop() is actually called.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 416 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that  Drop() is actually called.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 428 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 440 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 452 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 466 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is no call to Drop() and one call to Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 478 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is no call to Drop() and one call to Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 491 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is no call to Drop() and one call to Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 505 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is no call to Drop() and one call to Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 541 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 553 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 566 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there are no calls to Drop() or Create() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 580 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is a call to Alter() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 594 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You need to test (assert) that there is a call to Alter() here.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 600 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Change all It-blocks that "throws" so that their description include 'Should throw the correct error...'

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 656 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Maybe use the same description as the one before instead of  'PasswordValidationFailed'.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 669 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Se previous comment. Maybe write out 'PasswordChangeFailed'?

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 682 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
This It-block is a duplicate to the one before?

No, but I'm not sure how to describe it differently because this part of the catch is the catch-all.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 695 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

Should throw when creating a SQL Login fails

Maybe 'Should throw the correct error when SQL login cannot be created'?

Done.

Comments from Reviewable

Awesome getting 100% test code coverage! Just a few more comments on the code and tests then I think we are good to go.

Reviewed 6 of 6 files at r4.
Review status: all files reviewed at latest revision, 25 unresolved discussions.

CHANGELOG.md, line 24 at r4 (raw file):

  - Removed ShouldProcess statements
  - Added the ability to enforce password policies on SQL logins
  -

You seem to have an empty item here

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 395 at r4 (raw file):

}

# Create a function to alter the login. This allows us to more easily write mocks.

Please add comment-based help to this function

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 396 at r4 (raw file):

# Create a function to alter the login. This allows us to more easily write mocks.
function AlterLogin

Please use a name in the formVerb-Noun on this function. Also needs to be an approved verb (see Get-Verb). I guess Set-Login or Set-SQLServerLogin might be appropriate?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 398 at r4 (raw file):

function AlterLogin
{
    Param

param (lowercase 'p')

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 405 at r4 (raw file):

    )

    $Login.Alter()

We don't need any error handling here?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 408 at r4 (raw file):

}

# Create a function to create the login. This allows us to more easily write mocks.

Please add comment-based help to this function

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 409 at r4 (raw file):

# Create a function to create the login. This allows us to more easily write mocks.
function CreateLogin

Please use a name in the formVerb-Noun on this function.  Create-Login or Create-SQLServerLogin?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 412 at r4 (raw file):

{
    [CmdletBinding(DefaultParameterSetName = 'WindowsLogin')]
    Param

param (lowercase 'p')

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 415 at r4 (raw file):

    (
        [Parameter(Mandatory = $true, ParameterSetName = 'WindowsLogin')]
        [Parameter(ParameterSetName = 'SqlLogin')]

Shouldn't this be mandatory for SqlLogin as well?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 423 at r4 (raw file):

        $LoginCreateOptions,

        [Parameter(ParameterSetName = 'SqlLogin')]

Shouldn't this be mandatory as well? Will it work if this is $null?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 453 at r4 (raw file):

        }

        'WindowsLogin' { $login.Create() }

Please make the braces like 'SqlLogin', on seperate rows.
We don't need any error handling here?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 467 at r4 (raw file):

    )

    $Login.Drop()

We don't need any error handling here?

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 470 at r4 (raw file):

}

# Create a function to set the password. This allows us to more easily write mocks.

Please add comment-based help to this function

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 471 at r4 (raw file):

# Create a function to set the password. This allows us to more easily write mocks.
function SetPassword

Please use a name in the formVerb-Noun on this function.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 473 at r4 (raw file):

function SetPassword
{
    Param

param (lowercase 'p')

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 746 at r4 (raw file):

                    $login.LoginType = 'WindowsUser'

                    AlterLogin -Login $login

You mean it should not throw an error? Maybe add a | Should Not Throw then maybe?
It would also be good that the mocked method Alter() in the SMO.cs actually throws an error. You could add  new properties to the Login class that says MockName and MockLoginType, and if those does not match the property Name and LoginType when method  Alter()is called, the method throws an error. Maybe you need to mock other properties to verify this. I just took an example.
My point is to test that those properties we alter is what we actually alter, and the code did not change anything in between. :)

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 759 at r4 (raw file):

                    $login.LoginType = 'WindowsUser'

                    CreateLogin -Login $login

Same as previous. Make sure what we create is what we wanted to create.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 772 at r4 (raw file):

                    }

                    CreateLogin @createLoginParams

Same as previous. Make sure what we create is what we wanted to create.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 824 at r4 (raw file):

                    $login.LoginType = 'WindowsUser'

                    DropLogin -Login $login

Same as previous. Make sure what we drop is what we wanted to drop.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 832 at r4 (raw file):

            Mock -CommandName New-TerminatingError -MockWith { $ErrorType } -ModuleName $script:DSCResourceName

            Context 'When the password is set on an existing login' {

Please add a test where the password is set correctly and does not throw, and also same as previous, that we make sure what we set the password that we wanted to set.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 863 at r4 (raw file):

    }

    Describe "$($script:DSCResourceName)\Set-TargetResource" {

I think you got a copy/paste error her?  :smile:

Comments from Reviewable

Review status: 3 of 9 files reviewed at latest revision, 25 unresolved discussions.

CHANGELOG.md, line 24 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You seem to have an empty item here

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 204 at r3 (raw file):
Previously, randomnote1 (Dan Reist) wrote…

hrow New-TerminatingError -ErrorType PasswordChangeFailed -FormatArgs $Name -ErrorCategory NotSpec

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 395 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help to this function

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 396 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please use a name in the formVerb-Noun on this function. Also needs to be an approved verb (see Get-Verb). I guess Set-Login or Set-SQLServerLogin might be appropriate?

For functions internal to a module I didn't think it mattered. Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 398 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
param (lowercase 'p')

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 405 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We don't need any error handling here?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 408 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help to this function

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 409 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please use a name in the formVerb-Noun on this function.  Create-Login or Create-SQLServerLogin?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 412 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
param (lowercase 'p')

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 415 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Shouldn't this be mandatory for SqlLogin as well?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 423 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Shouldn't this be mandatory as well? Will it work if this is $null?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 453 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please make the braces like 'SqlLogin', on seperate rows.
We don't need any error handling here?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 467 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
We don't need any error handling here?

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 470 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add comment-based help to this function

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 471 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please use a name in the formVerb-Noun on this function.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 473 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
param (lowercase 'p')

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 746 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
You mean it should not throw an error? Maybe add a | Should Not Throw then maybe?
It would also be good that the mocked method Alter() in the SMO.cs actually throws an error. You could add  new properties to the Login class that says MockName and MockLoginType, and if those does not match the property Name and LoginType when method  Alter()is called, the method throws an error. Maybe you need to mock other properties to verify this. I just took an example.
My point is to test that those properties we alter is what we actually alter, and the code did not change anything in between. :)

Done. I think I'm in the ballpark here. Let me know if you think I should expand on this.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 759 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous. Make sure what we create is what we wanted to create.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 772 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous. Make sure what we create is what we wanted to create.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 824 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same as previous. Make sure what we drop is what we wanted to drop.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 832 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Please add a test where the password is set correctly and does not throw, and also same as previous, that we make sure what we set the password that we wanted to set.

Added the set correctly test. I think the second scenario is covered the best that it can be. There's not really a property on the object that can be checked to validate if the password is correctly set.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 863 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think you got a copy/paste error her?  :smile:

Done.

Comments from Reviewable

Reviewed 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 426 at r5 (raw file):

$ErrorActionPreference = 'Continue'

Should be $ErrorActionPreference = $originalErrorActionPreference ?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 682 at r3 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
No, but I'm not sure how to describe it differently because this part of the catch is the catch-all.

Okey, what got me is that both is evaluation 'LoginCreationFailed'. Maybe throw another error message in the helper function for the catch-all to distinguish both errors?

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 746 at r4 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done. I think I'm in the ballpark here. Let me know if you think I should expand on this.

You got it! Nicely done! This was what I was thinking. It gives at least a check that nothing was changed on the way to the mock function. If it needs expanding, let's do that in the future.
LGTM

Comments from Reviewable

Review status: 4 of 9 files reviewed at latest revision, 5 unresolved discussions.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 204 at r3 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done.

Done.

DSCResources/MSFT_xSQLServerLogin/MSFT_xSQLServerLogin.psm1, line 426 at r5 (raw file):
Previously, johlju (Johan Ljunggren) wrote…

$ErrorActionPreference = 'Continue'

Should be $ErrorActionPreference = $originalErrorActionPreference ?

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 669 at r3 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done.

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 682 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Okey, what got me is that both is evaluation 'LoginCreationFailed'. Maybe throw another error message in the helper function for the catch-all to distinguish both errors?

Done.

Tests/Unit/MSFT_xSQLServerLogin.Tests.ps1, line 772 at r4 (raw file):
Previously, randomnote1 (Dan Reist) wrote…
Done.

Done.

Comments from Reviewable

Reviewed 5 of 5 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@randomnote1 This LGTM now. But I see very strange behaviour in Reviewable from your changes now when I look back. In the pushes you done, I see a lot of "green" changes that is already merge from other PR's. I normally does not look like this. So may I ask how you do rebase, do you follow the steps outlined in the following guide?
For reference this is how rebase should be done:
https://github.com/PowerShell/DscResources/blob/master/GettingStartedWithGitHub.md#resolve-merge-conflicts

@randomnote1 I referring to README.md and CHANGELOG.md that looks like you might have copy/pasted in already merge rows into your commit to resolve merge conflicts and then commited those to your branch? Could this be so? Just try to figure this one out. 😄

That is the process I followed. The last couple revisions I did a rebase before I pushed each of them because there were merge conflicts in CHANGELOG.md.
git checkout dev
git fetch origin dev
git rebase origin/dev
git push my dev --force
git checkout xSQLServerLogin-RemoveShouldProcess
git rebase my/dev
git push xSQLServerLogin-RemoveShouldProcess --force

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

yup, probably readme too

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

And when you have a merge conflict you fix the conflict, save the file, and add the file, the do 'rebase continue'?
Like this:
1. Change file for conflicts
2. Save the file
git add <file>      # Do this for each file that you fixed merged conflicts in. I.e 'git add README.md'.
git rebase --continue

At point 2, you did not add a commit message and commited?

That is correct.

Okay... Strange. 😄
But all good, the end result will be correct. So I merge this one now.
Thanks my friend! Awesome work! 👍

I've been using Visual Studio Code to add the files (git add <file>) after the conflict has been resolved. After that I manually entered the git rebase continue

@randomnote1 Yes, that is how I do it to. That is all good.

Awesome, thanks!

Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

When I tested rebase my working branch I see that something unusual did happen. Not a problem, but still curious.

I've been using Visual Studio Code to add the files (git add ) after the conflict has been resolved.

When you sa you add the file in Code. After you resolved the conflict, and saved the file.

Do you press the plus sign, to stage the file, then run 'git rebase --continue'?

Numer 1 is the correct one.

@randomnote1 forgot to tag you, please see previous comment. 😄

@johlju , I normally follow option Number 1, though I suppose it's possible I clicked commit. Good question!

@randomnote1 Don't really know if that would  cause that either.  But it was a good thing that it happened, and with a limited number of rows that was affected. It was easy to verify. Because of that I learned a new thing. I will look out for this in all future PR's before merging. :smile: