This repository was archived by the owner on Aug 18, 2020. It is now read-only.

[DEVOPS-1131] automated proposal scripting #3916

Merged

iohk-bors merged 8 commits into develop from devops-1131

Jan 31, 2019

Contributor

cleverca22 commented Nov 30, 2018

Description

Linked issue

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

How to merge

Send the message bors r+ to merge this PR. For more information, see
docs/how-to/bors.md.

cleverca22 added the wip label

erikd reviewed

View reviewed changes

cluster/demo/Main.hs Outdated

@@ @@ -73,6 +73,7 @@ main = void $ do @@
                   handles <- forM cluster $ \case
                       RunningCoreNode (NodeName nodeId) env handle -> do
+                          -- todo, dont mapm over chars

Member

erikd Nov 30, 2018

Sorry, I do not understand that comment.

Contributor Author

cleverca22 Nov 30, 2018

putTextLn will run mapM over the string, and print it a single byte at a time, which wastes cpu and heavily interleaves the words when running in parallel

Member

erikd Nov 30, 2018

putStrLn does that for String, but i suspect putTextLn does something smarter.

Contributor Author

cleverca22 Dec 13, 2018

i added the comment after observing that kind of interleaving on the console output

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+              {-# LANGUAGE GeneralizedNewtypeDeriving   #-}
+              {-# LANGUAGE OverloadedStrings #-}
+              {-# LANGUAGE RankNTypes #-}
+              {-# LANGUAGE RecordWildCards #-}

Member

erikd Nov 30, 2018

Probably want to drop that extension.

In some situations its fine, but in others its a real hinderance to readability and understandability and there is no way to tell the compiler to allow the relatively benign uses and disallow the bad uses.

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+              {-# LANGUAGE RankNTypes #-}
+              {-# LANGUAGE RecordWildCards #-}
+              {-# LANGUAGE TypeApplications #-}
+              {-# LANGUAGE NamedFieldPuns #-}

Member

erikd Nov 30, 2018

We mentioned that in Slack. If its not needed any longer, its probably best to remove it (same with other extensions)

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+              {-# LANGUAGE TypeApplications #-}
+              {-# LANGUAGE NamedFieldPuns #-}
+              module AutomatedTestRunner (Example, getGenesisConfig, loadNKeys, doUpdate, onStartup, on, getScript, runScript, NodeType(..), startNode) where

Member

erikd Nov 30, 2018 •

edited

Loading

Probably need to run stylish-haskell on that.

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+                , sbEpochSlots :: SlotCount
+                , sbGenesisConfig :: Config
+                }
+              data NodeType = Core { ntIdex :: Integer }

Member

erikd Nov 30, 2018

If a data type has a single file, it should be a newtype :).

Contributor Author

cleverca22 Nov 30, 2018

it will become a sum type soon, to support relay and maybe wallet nodes

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+                runRealMode updateConfiguration genesisConfig txpConfig nr thing2
+              workers :: (HasConfigurations, TestScript a) => Genesis.Config -> InputParams2 a -> [ (Text, Diffusion AuxxMode -> AuxxMode ()) ]
+              workers genesisConfig InputParams2{ip2EventChan,ip2Script,ip2ReplyChan} =

Member

erikd Nov 30, 2018

I would actually write that as:

workers genesisConfig (InputParams2 ip2EventChan ip2Script ip2ReplyChan) =

which i thing is a little less noisy. I would also tend to use shorter names for what are local variables.

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+              getGenesisConfig :: Example Config
+              getGenesisConfig = do
+                oldsb <- get
+                pure $ sbGenesisConfig oldsb

Member

erikd Nov 30, 2018

I would write that as:

   sbGenesisConfig <$> get

to remove a local variable.

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+                    sbScript = script
+                  }
+                put newsb
+                pure ()

Member

erikd Nov 30, 2018

The type of put newsb should probably be Example () so the pure () can be dropped.

Contributor Author

cleverca22 Nov 30, 2018

it already is, the pure () was just a left-over, from when the function was entirely empty and ghc complained about an empty do block

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+              startNode :: NodeType -> IO NodeHandle
+              startNode (Core idx) = do
+                let
+                  _params = [ "--configuration-file", "../lib/configuration.yaml"

Member

erikd Nov 30, 2018

Redundant?

Contributor Author

cleverca22 Nov 30, 2018

that code isnt written yet

Member

erikd Nov 30, 2018

Ah ok, fair enough. If you put:

-- TODO: ....

then i won't ask about it :)

erikd reviewed

View reviewed changes

script-runner/AutomatedTestRunner.hs Outdated

+                           , "--keyfile", "poc-state/secret" <> (show idx) <> ".key"
+                           ]
+                later <- async $ do
+                  pure ()

Member

erikd Nov 30, 2018

Wat?

Contributor Author

cleverca22 Nov 30, 2018 •

edited

Loading

also unfinished code, its supposed to spawn a node, and block until it exits
i just needed a dummy ASync for now, so the return type is right, for when that does get done

erikd reviewed

View reviewed changes

script-runner/BlockParser.hs

+              printBlock :: FilePath -> IO ()
+              printBlock filename = do
+                raw <- LBS.readFile filename

Member

erikd Nov 30, 2018

Should probably avoid lazy ByteString especially for reading a file. Lazy I/O is a bit a of a trap.

If you need raw to be an LBS; then maybe (untested):

     raw <- LBS.fromChunks . (:[]) <$> BS.readFile filename

erikd reviewed

View reviewed changes

script-runner/BlockParser.hs

+                let
+                  blockraw :: LBS.ByteString
+                  _undoraw :: LBS.ByteString
+                  Right ("", (blockraw, _undoraw)) = deserialiseFromBytes decode raw

Member

erikd Nov 30, 2018 •

edited

Loading

Rather than the pattern match you should try (not tested but something like this should work):

    Right ("", blockraw) = fst <$> deserialiseFromBytes decode raw

Contributor Author

cleverca22 Nov 30, 2018

this whole file is more of a demo on how to parse blocks rather then code intended to be used in production, so i was letting the pattern match be a bit more messy

erikd reviewed

View reviewed changes

script-runner/BrickUITypes.hs Outdated

+              data CustomEvent = CESlotStart SlotStart
+                  | CENodeInfo NodeInfo
+                  | QuitEvent deriving Show

Member

erikd Nov 30, 2018

I would format that as:

data CustomEvent
    = CESlotStart SlotStart
    | CENodeInfo NodeInfo
    | QuitEvent
    deriving Show

erikd reviewed

View reviewed changes

script-runner/Poc.hs Outdated

+              main :: IO ()
+              main = do
+                _corenodes <- forM (range (0,3)) $ \node -> startNode (Core node)

Member

erikd Nov 30, 2018

??

Contributor Author

cleverca22 Nov 30, 2018

unfinished code, it should perform a rolling restart of the core nodes, using the handles startNode returned, at a future point in time

cleverca22 force-pushed the devops-1131 branch from 752819f to d20b5c0 Compare

November 30, 2018 03:10

erikd approved these changes

View reviewed changes

Member

erikd left a comment

LGTM! Probably want to manually clean up the commit history before letting bors merge it.

cleverca22 force-pushed the devops-1131 branch from bf314d8 to 81f8f8e Compare

December 1, 2018 04:58

cleverca22 force-pushed the devops-1131 branch 4 times, most recently from 9c7c305 to e5aa356 Compare

December 18, 2018 17:16

cleverca22 force-pushed the devops-1131 branch 2 times, most recently from 88ee923 to c529884 Compare

January 16, 2019 22:47

mhuesch suggested changes

View reviewed changes

Contributor

mhuesch left a comment

Looks great for a first version. I noted a few quibbles, nothing major, but some questions.

lib/src/Pos/Launcher/Configuration.hs Outdated Show resolved Hide resolved

lib/src/Pos/Launcher/Runner.hs Show resolved Hide resolved

script-runner/BlockParser.hs Outdated Show resolved Hide resolved

script-runner/Makefile Outdated Show resolved Hide resolved

script-runner/TestCases.hs Show resolved Hide resolved

Contributor

mhuesch commented Jan 17, 2019 •

edited

Loading

@cleverca22 I just pushed a commit which modifies variable names to be hopefully more descriptive. It's hard to figure out exactly what's going on, though 🤷‍♂️ (due to the complexity of auxx, not from your changes)

cleverca22 force-pushed the devops-1131 branch from 0f776d0 to ae217c7 Compare

January 22, 2019 06:09

Contributor Author

cleverca22 commented Jan 22, 2019

bors try

iohk-bors bot added a commit that referenced this pull request


          Try #3916:

d9a2e9d

Contributor

iohk-bors bot commented Jan 22, 2019

try

Build failed

buildkite/cardano-sl

mhuesch approved these changes

View reviewed changes

mhuesch force-pushed the devops-1131 branch from db0896d to 0338836 Compare

January 24, 2019 01:31

input-output-hk deleted a comment from iohk-bors bot

Contributor

mhuesch commented Jan 24, 2019

@cleverca22 I rebased, fixed the merge type-errors, fixed some compiler complaints, and force-pushed. This should pass CI / satisfy bors.

mhuesch force-pushed the devops-1131 branch from 64c4bc1 to fcb38b0 Compare

January 30, 2019 21:21

cleverca22 and others added 6 commits

January 31, 2019 10:19


          [DEVOPS-1131] scriptable cluster test automation

1b9b76b


          [DEVOPS-1131] Add (hopefully) more descriptive variable names

d43d96b


          [DEVOPS-1131] remove paths specific to my local machine

df67a2a


          [DEVOPS-1131] add some scripts that can run the tests under stack

f477203


          [DEVOPS-1131] Add README, tweak scripts, add gitignore entry

8709a1a


          [DEVOPS-1131] Fix merge errors & compiler complaints

1dac017

cleverca22 force-pushed the devops-1131 branch 2 times, most recently from ce7352b to 315b968 Compare

January 31, 2019 14:36


          [DEVOPS-1131] Relocate generators & roundtrip tests

3adf047

cleverca22 force-pushed the devops-1131 branch from 315b968 to 3adf047 Compare

January 31, 2019 16:09


          [DEVOPS-1131] Use JSON+YAML roundtrip functions

269b160

cleverca22 removed the wip label

Contributor

mhuesch commented Jan 31, 2019

bors r+

Contributor

disassembler commented Jan 31, 2019

bors p=1

iohk-bors bot added a commit that referenced this pull request


          Merge #3916 #4050 #4054

da0e177

3916: [DEVOPS-1131] automated proposal scripting r=mhuesch a=cleverca22

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

## Linked issue

<!--- Put here the relevant issue from YouTrack -->



4050: Internal Endpoints Disclaimer r=KtorZ a=KtorZ

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

As a follow-up from the discussion in cardano-foundation/cardano-wallet#151 (comment), we will add a proper disclaimer in the documentation about the internal endpoints. 

## Linked issue

<!--- Put here the relevant issue from YouTrack -->

cardano-foundation/cardano-wallet#228



4054: Update CHANGELOG about changes in 1.5 (for the wallet backend) r=KtorZ a=KtorZ

## Description

<!--- A brief description of this PR and the problem is trying to solve -->

## Linked issue

<!--- Put here the relevant issue from YouTrack -->



Co-authored-by: Michael Bishop <[email protected]>
Co-authored-by: Michael Hueschen <[email protected]>
Co-authored-by: KtorZ <[email protected]>

Contributor

iohk-bors bot commented Jan 31, 2019

Build succeeded

iohk-bors bot merged commit 269b160 into develop

iohk-bors bot deleted the devops-1131 branch

January 31, 2019 21:09

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet